Implement trace, trace_span, span_context and reporters by liyanhui1228 · Pull Request #1 · census-instrumentation/opencensus-python

liyanhui1228 · 2017-07-25T01:35:36Z

Mostly reviewed in google-cloud-python repository, just changed the namespace to opencensus.trace and added unit tests and reporters.

duggelz · 2017-07-25T19:19:30Z

trace/.gitignore

@@ -0,0 +1,66 @@
+*.py[cod]


Can we get rid of this file since it's the same as the .gitignore at the top level?

duggelz · 2017-07-25T19:20:23Z

trace/file_name

@@ -0,0 +1 @@
+{}


Spurious file?

duggelz · 2017-07-25T19:20:48Z

trace/README.rst

@@ -0,0 +1,12 @@
+OpenCensus - A stats collection and distributed tracing framework


Can we merge this with the top-level README.rst?

This file is for opencensus-trace, and the top level is for opencensus overall. I'll add sample usage after merging the tracers stuff.

duggelz · 2017-07-25T19:21:17Z

trace/nox.py~

@@ -0,0 +1,79 @@
+# Copyright 2016 Google Inc.


Spurious file

duggelz · 2017-07-25T19:31:39Z

trace/opencensus/trace/reporters/print_reporter.py

+        :returns: Traces printed.
+        """
+        print(traces)
+        return traces


Why return anything?

It's for unit testing this function, to assert the content it prints.

That's fine. If we move to pytest there is an easier way.

duggelz · 2017-07-25T19:50:37Z

trace/opencensus/trace/span_context.py

+                self.from_header = False
+                return generate_trace_id()
+
+        except TypeError:


I'd prefer that a type be an assertion failure than implicitly creating a new trace id, because it indicates a bug in someone's code.

duggelz · 2017-07-25T19:53:26Z

trace/opencensus/trace/trace_span.py

+
+"""TraceSpan for sending traces to the Stackdriver Trace API."""
+
+from datetime import datetime


Import ordering

duggelz · 2017-07-25T19:54:51Z

trace/opencensus/trace/trace.py

+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+"""Trace for interacting with the Stackdriver Trace API."""


Can you update this comment?

duggelz · 2017-07-25T19:56:18Z

trace/opencensus/trace/trace_span.py

+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+"""TraceSpan for sending traces to the Stackdriver Trace API."""


Can you remove the reference to Stackdriver? You describe what a TraceSpan is in the class, so maybe this comment isn't needed at all.

duggelz · 2017-07-25T19:57:42Z

trace/requirements.txt

@@ -0,0 +1,2 @@
+googleapis-common-protos>=1.5.2, <2.0dev


These aren't required right now

Creating stats "bare bone" prototype

liyanhui1228 added 2 commits July 24, 2017 18:29

Implementation for trace, trace_span, span_context and reporters

57fce84

Fix style

bf621a1

liyanhui1228 requested review from bogdandrutu and duggelz July 25, 2017 01:45

duggelz suggested changes Jul 25, 2017

View reviewed changes

Address comments

7a34ea8

duggelz approved these changes Jul 26, 2017

View reviewed changes

Change to use assert instead of raising TypeError

27cd3a2

liyanhui1228 merged commit e007e5d into census-instrumentation:master Jul 26, 2017

liyanhui1228 deleted the trace branch July 28, 2017 17:31

kcooperstein added a commit that referenced this pull request Jun 4, 2018

Merge pull request #1 from kcooperstein/stats

511518c

Creating stats "bare bone" prototype

zoidyzoidzoid mentioned this pull request Nov 14, 2018

Drop negative measurements on record #389

Merged

Gamecock mentioned this pull request Dec 16, 2020

Errors with large URLs #988

Closed

		@@ -0,0 +1,12 @@
		OpenCensus - A stats collection and distributed tracing framework


		"""TraceSpan for sending traces to the Stackdriver Trace API."""

		from datetime import datetime

		@@ -0,0 +1 @@
		{} No newline at end of file

Comments

Conversation

liyanhui1228 commented Jul 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

liyanhui1228 commented Jul 25, 2017 •

edited

Loading