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: add IsRemote field to SpanContext, set by propagators #451

Merged
merged 5 commits into from
Oct 24, 2019

Conversation

KaindlJulian
Copy link
Contributor

Which problem is this PR solving?

Short description of the changes

  • Add optional isRemote to SpanContext
  • Set it to true when extracting a propagated SpanContext

@KaindlJulian KaindlJulian changed the title Add IsRemote field to SpanContext, set by propagators feat: add IsRemote field to SpanContext, set by propagators Oct 22, 2019
@codecov-io
Copy link

codecov-io commented Oct 22, 2019

Codecov Report

Merging #451 into master will increase coverage by 0.29%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #451      +/-   ##
==========================================
+ Coverage   94.94%   95.23%   +0.29%     
==========================================
  Files         123      125       +2     
  Lines        5815     6279     +464     
  Branches      491      516      +25     
==========================================
+ Hits         5521     5980     +459     
- Misses        294      299       +5
Impacted Files Coverage Δ
...lemetry-core/test/context/HttpTraceContext.test.ts 100% <ø> (ø) ⬆️
...metry-core/test/context/BinaryTraceContext.test.ts 100% <ø> (ø) ⬆️
...s/opentelemetry-core/test/context/B3Format.test.ts 100% <100%> (ø) ⬆️
...telemetry-core/src/context/propagation/B3Format.ts 96% <100%> (-0.43%) ⬇️
...core/src/context/propagation/BinaryTraceContext.ts 96.72% <100%> (-0.96%) ⬇️
...y-core/src/context/propagation/HttpTraceContext.ts 100% <100%> (ø) ⬆️
...opentelemetry-core/src/platform/node/timer-util.ts 33.33% <0%> (-16.67%) ⬇️
...ges/opentelemetry-core/src/common/ConsoleLogger.ts 91.66% <0%> (-1.67%) ⬇️
...entelemetry-core/test/common/ConsoleLogger.test.ts 100% <0%> (ø) ⬆️
... and 28 more

@mayurkale22
Copy link
Member

Thank you so much for the contribution and welcome to OpenTelemetry 👍

@mayurkale22
Copy link
Member

Should we also use this flag (isRemote) to set span.shared when send/transform it to Zipkin's Span format? Currently, it is set to false by default.

/**
* True if we are contributing to a span started by another tracer (ex on a
* different host).
*/
shared?: boolean;

/cc @hekike

@mayurkale22
Copy link
Member

We have to set the flag when extracing from BinaryTraceContext - used by gRPC plugin, Could you please update the PR and include that?

@@ -35,6 +35,10 @@ export interface SpanContext {
* lowercase hex characters corresponding to 64 bits.
*/
spanId: string;
/**
* Indicates if the SpanContext was propagated from a remote parent.
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you specify the default? I assume the default is false

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not set to false anywhere explicitly so it'll be undefined by default. Should I just change the comment or add a isRemote: false to all new SpanContexts?

mayurkale22 and others added 2 commits October 23, 2019 11:20
Makes it more obvious when the flag changes and what the default value is.
@mayurkale22
Copy link
Member

Should we also use this flag (isRemote) to set span.shared when send/transform it to Zipkin's Span format? Currently, it is set to false by default.

LGTM, merging now. Let's consider above stuff for separate PR/Issue.

@mayurkale22 mayurkale22 merged commit 1975647 into open-telemetry:master Oct 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add IsRemote field to SpanContext, set by propagators
5 participants