Skip to content
This repository was archived by the owner on Sep 17, 2025. It is now read-only.

Allow user to instrument a library without enabling tracing (using the NoopTracer)#285

Merged
mayurkale22 merged 8 commits intocensus-instrumentation:masterfrom
mayurkale22:noop-tracer-enhancement-issues-277
Sep 8, 2018
Merged

Allow user to instrument a library without enabling tracing (using the NoopTracer)#285
mayurkale22 merged 8 commits intocensus-instrumentation:masterfrom
mayurkale22:noop-tracer-enhancement-issues-277

Conversation

@mayurkale22
Copy link
Copy Markdown
Member

No description provided.

@mayurkale22
Copy link
Copy Markdown
Member Author

#277

…-python into noop-tracer-enhancement-issues-277
@songy23 songy23 requested a review from liyanhui1228 August 27, 2018 21:21
Comment thread opencensus/trace/noopspan.py Outdated
from opencensus.trace.tracers import base


class NoopSpan(object):
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In java we call this BlankSpan, because people may use this even for a request that they don't want to trace.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed NoopSpan to BlankSpan...

@mayurkale22
Copy link
Copy Markdown
Member Author

/cc @liyanhui1228 for review

@liyanhui1228
Copy link
Copy Markdown
Contributor

liyanhui1228 commented Sep 7, 2018

Is the NoopSpan is an improvement of the NullContextManager? If so, we could remove the NullContextManager class as it won't be used anywhere else. This could be done in a separate PR. And be sure to sync up this branch with master branch before merging.

…-python into noop-tracer-enhancement-issues-277
…kale22/opencensus-python into noop-tracer-enhancement-issues-277
@mayurkale22
Copy link
Copy Markdown
Member Author

@liyanhui1228 Thanks for the review

@odeke-em
Copy link
Copy Markdown
Member

Thanks for the fix @mayurkale22 and @bogdandrutu @liyanhui1228 for the review! I just have one minor nit: in the future please refer to issues in the commit message so that Github can auto-close them on merge to the main branch e.g.

trace: make a NoopTracer that drops spans instead of crashing

....[Rest of message]....

Fixes #277

that way I'd have gotten the notification 11 days ago on it being closed with a PR instead of almost 2 weeks later :)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants