Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(tracer): expose bind method from scope manager #201

Merged
merged 1 commit into from Aug 15, 2019

Conversation

@vmarchaud
Copy link
Member

commented Aug 14, 2019

As discussed in #161, we need the tracer to expose the bind method from the scope manager to allow plugins to use it.
see #161 (comment)

I've also updated getCurrentSpan on the basic tracer to actually return a NoopSpan when no span is set in the current scope (otherwise we will have some crash if the context is loss).

@vmarchaud vmarchaud force-pushed the vmarchaud:tracer-with-bind branch from 55f036f to 63720b8 Aug 14, 2019

@codecov-io

This comment has been minimized.

Copy link

commented Aug 14, 2019

Codecov Report

Merging #201 into master will decrease coverage by 0.37%.
The diff coverage is 74.35%.

@@            Coverage Diff             @@
##           master     #201      +/-   ##
==========================================
- Coverage   93.92%   93.54%   -0.38%     
==========================================
  Files          47       47              
  Lines        1596     1658      +62     
  Branches      101      102       +1     
==========================================
+ Hits         1499     1551      +52     
- Misses         97      107      +10
Impacted Files Coverage Δ
...entelemetry-core/test/trace/TracerDelegate.test.ts 100% <ø> (ø) ⬆️
.../opentelemetry-node-tracer/test/NodeTracer.test.ts 0% <0%> (ø) ⬆️
...s/opentelemetry-core/test/trace/NoopTracer.test.ts 100% <100%> (ø) ⬆️
...pentelemetry-basic-tracer/test/BasicTracer.test.ts 100% <100%> (ø) ⬆️
...ges/opentelemetry-core/src/trace/TracerDelegate.ts 100% <100%> (ø) ⬆️
...ackages/opentelemetry-core/src/trace/NoopTracer.ts 100% <100%> (ø) ⬆️
...ages/opentelemetry-basic-tracer/src/BasicTracer.ts 94.44% <83.33%> (-1.48%) ⬇️
...kages/opentelemetry-basic-tracer/test/Span.test.ts 100% <0%> (ø) ⬆️
...telemetry-scope-base/test/NoopScopeManager.test.ts 100% <0%> (ø) ⬆️

@vmarchaud vmarchaud force-pushed the vmarchaud:tracer-with-bind branch from 63720b8 to e5b4802 Aug 14, 2019

@vmarchaud

This comment has been minimized.

Copy link
Member Author

commented Aug 14, 2019

The loss in coverage is normal, the BasicTracer should handle the fact that the scope manager can return null and return NoopSpan in this case.
It's only using the AsyncHooksScopeManager that we can cover the other branch where the span returned is not null.

However i wonder if returning an invalid span is a good idea here. Valid span can be created from an invalid one i think, which i pretty sure is a bad idea.

@vmarchaud vmarchaud force-pushed the vmarchaud:tracer-with-bind branch 3 times, most recently from e00d603 to b99c2cd Aug 14, 2019

@vmarchaud vmarchaud force-pushed the vmarchaud:tracer-with-bind branch 2 times, most recently from 40dbf58 to 55c7c60 Aug 14, 2019

@vmarchaud

This comment has been minimized.

Copy link
Member Author

commented Aug 14, 2019

Following discussion in today SIG (14/08/2019), getCurrentSpan should return null to avoid confusiong between NoopSpan (if no span is found in context) or a real Span. Updating my PR to reflect this behavior.

@vmarchaud vmarchaud force-pushed the vmarchaud:tracer-with-bind branch from 55c7c60 to be1a6cb Aug 14, 2019

@mayurkale22
Copy link
Contributor

left a comment

Added 2 comments, otherwise LGTM.

@vmarchaud vmarchaud force-pushed the vmarchaud:tracer-with-bind branch from be1a6cb to efb9ff4 Aug 14, 2019

@vmarchaud vmarchaud force-pushed the vmarchaud:tracer-with-bind branch from efb9ff4 to d3523ae Aug 15, 2019

@mayurkale22 mayurkale22 merged commit f36e44f into open-telemetry:master Aug 15, 2019

8 checks passed

ci/circleci: docs Your tests passed on CircleCI!
Details
ci/circleci: lint Your tests passed on CircleCI!
Details
ci/circleci: node10 Your tests passed on CircleCI!
Details
ci/circleci: node11 Your tests passed on CircleCI!
Details
ci/circleci: node12 Your tests passed on CircleCI!
Details
ci/circleci: node12-browsers Your tests passed on CircleCI!
Details
ci/circleci: node8 Your tests passed on CircleCI!
Details
cla/linuxfoundation vmarchaud authorized
Details

@danielkhan danielkhan added this to Done in Plugins Aug 28, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
4 participants
You can’t perform that action at this time.