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

chore: remove tracer apis not part of spec #1764

Merged
merged 13 commits into from
Dec 21, 2020

Conversation

Flarna
Copy link
Member

@Flarna Flarna commented Dec 17, 2020

This removes getCurrentSpan(), bind() and with() from Tracer because they are not required by spec and context operations are a concern of context not Tracer.

At least in NoopTracer the implementation of them was incorrect because context was not used.

This removes getCurrentSpan(), bind() and with() from Tracer because
they are not required by spec and context operations are a concern of
context not Tracer.

At least in NoopTracer the implementation of them was incorrect because
context was not used.
@codecov
Copy link

codecov bot commented Dec 17, 2020

Codecov Report

Merging #1764 (3866df0) into master (16a3316) will increase coverage by 0.04%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1764      +/-   ##
==========================================
+ Coverage   92.09%   92.13%   +0.04%     
==========================================
  Files         166      166              
  Lines        5591     5571      -20     
  Branches     1198     1197       -1     
==========================================
- Hits         5149     5133      -16     
+ Misses        442      438       -4     
Impacted Files Coverage Δ
packages/opentelemetry-api/src/trace/NoopTracer.ts 95.45% <ø> (-1.10%) ⬇️
...ackages/opentelemetry-api/src/trace/ProxyTracer.ts 93.75% <ø> (+11.14%) ⬆️
packages/opentelemetry-tracing/src/Tracer.ts 100.00% <ø> (ø)
packages/opentelemetry-api/src/api/global-utils.ts 100.00% <100.00%> (ø)
...ges/opentelemetry-instrumentation-http/src/http.ts 95.49% <100.00%> (ø)
...emetry-instrumentation-xml-http-request/src/xhr.ts 96.79% <100.00%> (+0.01%) ⬆️
packages/opentelemetry-plugin-fetch/src/fetch.ts 96.55% <100.00%> (+0.02%) ⬆️
...s/opentelemetry-plugin-grpc-js/src/client/utils.ts 93.25% <100.00%> (ø)
...-plugin-grpc-js/src/server/clientStreamAndUnary.ts 100.00% <100.00%> (ø)
...telemetry-plugin-grpc-js/src/server/patchServer.ts 88.52% <100.00%> (ø)
... and 4 more

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.

Looks good.

For those that don't know, the spec removed these methods from the tracer https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/trace/api.md#tracer and the only method left in spec is startSpan

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.

lgtm, i understand now why we should maybe namespace the context helpers since those methods has been removed

Copy link
Member

@obecny obecny 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 think we should provide a short guide in readme "how to upgrade" as withSpan function was very popular and was used widely so people might be confused how they should update code correctly. Would be nice to add this example in readme for some time being

context.with(setActiveSpan(context.active(), span) ...

@Flarna
Copy link
Member Author

Flarna commented Dec 18, 2020

@obecny Added a section in readme

Most likely it would be in general good to add something like this for all breaking changes.

@vmarchaud vmarchaud mentioned this pull request Dec 19, 2020
30 tasks
README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
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.

The API_BACKWARDS_COMPATIBILITY_VERSION should be bumped as this change is breaking.

@Flarna
Copy link
Member Author

Flarna commented Dec 21, 2020

The API_BACKWARDS_COMPATIBILITY_VERSION should be bumped as this change is breaking.

Should we do this on each PR or once per release?

By the way, we forgot to update this in 0.14.0 (incompatible at least because of #1734)

@dyladan
Copy link
Member

dyladan commented Dec 21, 2020

Should we do this on each PR or once per release?

Once per release should be fine I think

By the way, we forgot to update this in 0.14.0 (incompatible at least because of #1734)

👀 seems you're right. I'm working on making this use the current version and semver semantics at the moment so hopefully this will be automatic in the future.

@dyladan
Copy link
Member

dyladan commented Dec 21, 2020

Think this is mergeable when conflicts are fixed

@dyladan dyladan added the Merge:LGTM This PR is ready to be merged by a Maintainer (has enough valid approvals, successful build, etc.) label Dec 21, 2020
@dyladan dyladan merged commit a2304c9 into open-telemetry:master Dec 21, 2020
@dyladan dyladan deleted the rm-tracer-apis branch December 21, 2020 22:12
@Flarna Flarna mentioned this pull request Feb 4, 2021
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
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