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(api): propagate spanContext only using API #1456 #1527

Merged
merged 6 commits into from
Oct 1, 2020

Conversation

vmarchaud
Copy link
Member

@vmarchaud vmarchaud commented Sep 13, 2020

This one is a little bit tricky, since it was assume from the start that the API could only contains noop we put the context management utils into the core.

As you can see in this PR, we need to get the spanContext stored inside the context from the Noop implementation, there are two way do to this:
- duplicating the declaration of the key, one in the API's noop tracer and one inside the core for general usage.
- moving the key declaration inside the API, but while at it why not move them all ? Specially since they all use API interfaces.

Waiting for feedback before marking as non-draft !

Fixes #1456

@vmarchaud vmarchaud self-assigned this Sep 13, 2020
@vmarchaud vmarchaud force-pushed the propagate-context-api branch from c1f21f2 to 797ae44 Compare September 13, 2020 14:05
@codecov
Copy link

codecov bot commented Sep 13, 2020

Codecov Report

Merging #1527 into master will decrease coverage by 0.47%.
The diff coverage is 97.22%.

@@            Coverage Diff             @@
##           master    #1527      +/-   ##
==========================================
- Coverage   93.43%   92.95%   -0.48%     
==========================================
  Files          98      144      +46     
  Lines        3518     4332     +814     
  Branches      768      871     +103     
==========================================
+ Hits         3287     4027     +740     
- Misses        231      305      +74     
Impacted Files Coverage Δ
...y-core/src/context/propagation/HttpTraceContext.ts 100.00% <ø> (ø)
packages/opentelemetry-tracing/src/Tracer.ts 98.33% <83.33%> (ø)
packages/opentelemetry-api/src/context/context.ts 83.33% <100.00%> (ø)
packages/opentelemetry-api/src/trace/NoopTracer.ts 100.00% <100.00%> (ø)
...metry-core/src/context/propagation/B3Propagator.ts 100.00% <100.00%> (ø)
packages/opentelemetry-plugin-http/src/http.ts 97.81% <100.00%> (ø)
...ackages/opentelemetry-shim-opentracing/src/shim.ts 87.70% <100.00%> (ø)
...telemetry-tracing/src/export/BatchSpanProcessor.ts 91.93% <100.00%> (ø)
...elemetry-tracing/src/export/SimpleSpanProcessor.ts 83.33% <100.00%> (ø)
... and 75 more

@Flarna
Copy link
Member

Flarna commented Sep 14, 2020

I think the key shouldn't be duplicated. Having details like this in core results in further problems like various plugins depend on core.
As a result it's not possible to implement our own SDK but reuse plugins.
If the API is the entrypoint for plugins it should contain everything needed by them.

@dyladan
Copy link
Member

dyladan commented Sep 15, 2020

I was the one that originally put those context management functions in the SDK instead of API. The reason for this at the time was that I was thinking they were sdk-specific implementation details, but it has become clear since then that the API is not really that useful without them. I think they can be moved into the API.

@vmarchaud
Copy link
Member Author

I will update the PR to move them into the API then 👍

@vmarchaud vmarchaud force-pushed the propagate-context-api branch 3 times, most recently from 1431e14 to 0b482fc Compare September 19, 2020 13:58
@vmarchaud vmarchaud marked this pull request as ready for review September 19, 2020 14:04
@vmarchaud vmarchaud requested a review from obecny September 19, 2020 14:04
@vmarchaud vmarchaud added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Sep 19, 2020
@vmarchaud vmarchaud force-pushed the propagate-context-api branch from c241d6c to 603acec Compare September 23, 2020 17:10
@vmarchaud
Copy link
Member Author

I've rebased the PR

@vmarchaud vmarchaud force-pushed the propagate-context-api branch 2 times, most recently from 41c1fc1 to 3fd83fb Compare September 24, 2020 09:17
Copy link
Member

@mwear mwear left a comment

Choose a reason for hiding this comment

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

LGTM

@dyladan dyladan merged commit acaa074 into open-telemetry:master Oct 1, 2020
dyladan added a commit to dyladan/opentelemetry-js that referenced this pull request Sep 9, 2022
dyladan added a commit to dyladan/opentelemetry-js that referenced this pull request Sep 9, 2022
pichlermarc pushed a commit to dynatrace-oss-contrib/opentelemetry-js that referenced this pull request Dec 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

It should be possible to propagate context with only the API
5 participants