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

fix: change default propagator to match spec #1218

Merged

Conversation

jonahrosenblum
Copy link
Contributor

Which problem is this PR solving?

Short description of the changes

  • Change the default propagator in the SDK to match what the spec requires.

  • Modify unit tests to reflect this change of default propagator.

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Jun 18, 2020

CLA Check
The committers are authorized under a signed CLA.

Copy link
Member

@dyladan dyladan left a comment

Choose a reason for hiding this comment

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

LGTM I actually thought it was already like this

@codecov
Copy link

codecov bot commented Jun 18, 2020

Codecov Report

Merging #1218 into master will not change coverage.
The diff coverage is 50.00%.

@@           Coverage Diff           @@
##           master    #1218   +/-   ##
=======================================
  Coverage   92.32%   92.32%           
=======================================
  Files         122      122           
  Lines        3533     3533           
  Branches      714      714           
=======================================
  Hits         3262     3262           
  Misses        271      271           
Impacted Files Coverage Δ
...s/opentelemetry-tracing/src/BasicTracerProvider.ts 75.75% <50.00%> (ø)

@dyladan
Copy link
Member

dyladan commented Jun 18, 2020

The lint will be fixed by #1217

@@ -78,7 +83,9 @@ export class BasicTracerProvider implements api.TracerProvider {
register(config: SDKRegistrationConfig = {}) {
api.trace.setGlobalTracerProvider(this);
if (config.propagator === undefined) {
config.propagator = new HttpTraceContext();
config.propagator = new CompositePropagator({
Copy link
Member

Choose a reason for hiding this comment

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

Currently, HttpTraceContext is highlighted as a default propagator in the documentation (https://github.com/open-telemetry/opentelemetry-js/tree/master/packages/opentelemetry-core#built-in-propagators). Could you please update the same?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi, I added a new commit to change the documentation. Thanks for pointing this out.

@mayurkale22 mayurkale22 added enhancement New feature or request Merge:LGTM This PR is ready to be merged by a Maintainer (has enough valid approvals, successful build, etc.) labels Jun 19, 2020
@dyladan dyladan merged commit 298b19f into open-telemetry:master Jun 19, 2020
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.

5 participants