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

Separate context propagation (OTEP 66) #769

Merged
merged 31 commits into from
Feb 18, 2020

Conversation

dyladan
Copy link
Member

@dyladan dyladan commented Feb 6, 2020

This PR is absolutely massive. Sorry about that but there was no real great way to split it up and still have it build/pass tests

Fixes #773

Which problem is this PR solving?

  • Introduce a context mechanism which stores things like active span (will store correlations in the future)

OTEP 66: https://github.com/open-telemetry/oteps/blob/master/text/0066-separate-context-propagation.md
Spec: open-telemetry/opentelemetry-specification#424

Short description of the changes

API

  • API now depends on Scope Base which contains the context class (explanation below).
  • Update propagators interface to accept Context

Scope Managers

  • In the future these will need to be renamed to Context Managers (probably)
  • Scope managers now store Context instead of directly storing a span
  • Scope base contains the Context class
  • Scope managers now return the ROOT_CONTEXT when they would otherwise return null (disabled, no active scope, etc)

Core

  • Add a Context helper class with getters/setters for the context object

Propagators (core and jaeger)

  • Update to use new Context based interface

Plugins

  • Update to use new Context based propagators interface

@dyladan dyladan requested review from mayurkale22, obecny, vmarchaud and OlivierAlbertini and removed request for mayurkale22 February 6, 2020 15:16
@codecov-io
Copy link

codecov-io commented Feb 6, 2020

Codecov Report

Merging #769 into master will decrease coverage by 0.54%.
The diff coverage is 96.19%.

@@            Coverage Diff             @@
##           master     #769      +/-   ##
==========================================
- Coverage   92.93%   92.39%   -0.55%     
==========================================
  Files         233      239       +6     
  Lines       10545    12621    +2076     
  Branches     1028     1128     +100     
==========================================
+ Hits         9800    11661    +1861     
- Misses        745      960     +215
Impacted Files Coverage Δ
...emetry-exporter-prometheus/test/prometheus.test.ts 98.66% <ø> (+0.05%) ⬆️
packages/opentelemetry-metrics/src/types.ts 100% <ø> (ø)
...kages/opentelemetry-metrics/test/mocks/Exporter.ts 33.33% <ø> (-66.67%) ⬇️
packages/opentelemetry-metrics/src/Meter.ts 100% <100%> (+1.4%) ⬆️
...-metrics/test/export/ConsoleMetricExporter.test.ts 100% <100%> (ø) ⬆️
packages/opentelemetry-metrics/src/Metric.ts 85.18% <100%> (-0.97%) ⬇️
...pentelemetry-exporter-prometheus/src/prometheus.ts 92.07% <100%> (+0.07%) ⬆️
...ckages/opentelemetry-metrics/src/export/Batcher.ts 100% <100%> (ø)
packages/opentelemetry-metrics/test/Meter.test.ts 100% <100%> (ø) ⬆️
...ges/opentelemetry-metrics/src/export/Aggregator.ts 100% <100%> (ø)
... and 145 more

@dyladan dyladan force-pushed the context branch 2 times, most recently from a28ab75 to 4ec7d98 Compare February 6, 2020 15:53
@@ -45,6 +45,9 @@
"publishConfig": {
"access": "public"
},
"dependencies": {
Copy link
Member Author

Choose a reason for hiding this comment

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

API now depends on scope base because that is where context is. Context is a separate mechanism outside of the API and the SDK, but which is depended on by both.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe Context and the dummy implementation should be moved like it was done with a other parts recently?

Copy link
Member Author

Choose a reason for hiding this comment

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

I would be willing to move Context into it's own package. @mayurkale22 wdyt?

Copy link
Member

Choose a reason for hiding this comment

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

Still thinking on this, I will also check how other libraries are doing it.

Copy link
Member

Choose a reason for hiding this comment

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

When we started it we decided to move it back so it could be used by other projects. However since it's started to get heavily linked to the OT specs (and not storing generic context) i would be in favor to move it

Copy link
Member Author

Choose a reason for hiding this comment

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

@vmarchaud I'm sorry I don't understand, you would be in favor of context having it's own package or you would be in favor of moving it into core/api/other?

@dyladan dyladan marked this pull request as ready for review February 6, 2020 17:59
@mayurkale22
Copy link
Member

In order to keep PR short (off the top of head):

  • Keep DistributedContext related changes separate from this PR (AFAIK DistributedContext name changed to CorrelationContext but all other semantics are as it is).
  • format and Carrier enhancement can be decoupled from this one - at first glance, seems straightforward and easy to merge.
  • Maybe another PR just to add Context class.

WDYT?

@mayurkale22
Copy link
Member

btw, this is great work 💯

@dyladan
Copy link
Member Author

dyladan commented Feb 10, 2020

@open-telemetry/javascript-approvers I know this is a big change but I would appreciate some reviews here please

@dyladan dyladan mentioned this pull request Feb 12, 2020
46 tasks
@mayurkale22 mayurkale22 modified the milestones: Alpha v0.4, Beta Feb 12, 2020
Copy link
Member

@bg451 bg451 left a comment

Choose a reason for hiding this comment

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

okay I did a quick pass and everything looks 👍

Copy link
Member

@vmarchaud vmarchaud left a comment

Choose a reason for hiding this comment

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

Looks good to me. i think we can merge as-is and do refactor (like renaming to context-manager or moving the noop/interfaces inside API) on follow-up PR

@dyladan
Copy link
Member Author

dyladan commented Feb 12, 2020

Looks good to me. i think we can merge as-is and do refactor (like renaming to context-manager or moving the noop/interfaces inside API) on follow-up PR

I agree except that noop implementations won't ever be in the API in this case. Context is meant to be a standalone concept that the api and sdk both consume.

@vmarchaud
Copy link
Member

@dyladan Indeed, i mixed things ^^

Copy link
Member

@OlivierAlbertini OlivierAlbertini left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@mayurkale22 mayurkale22 left a comment

Choose a reason for hiding this comment

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

lgtm

@mayurkale22 mayurkale22 added the Merge:LGTM This PR is ready to be merged by a Maintainer (has enough valid approvals, successful build, etc.) label Feb 18, 2020
@dyladan dyladan merged commit 92b41bc into open-telemetry:master Feb 18, 2020
@dyladan dyladan deleted the context branch February 18, 2020 20:46
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
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 Merge:LGTM This PR is ready to be merged by a Maintainer (has enough valid approvals, successful build, etc.)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Separate context propagation from tracing (OTEP 66)
7 participants