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.

Sign up for GitHub

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

Jump to bottom

chore: remove usages of Span constructor #2458

Merged
merged 3 commits into from
Oct 2, 2024
Merged

chore: remove usages of Span constructor #2458

merged 3 commits into from
Oct 2, 2024
+44 −52

Conversation

david-luna
Copy link
Contributor

Which problem is this PR solving?

The Span constructor from ' @opentelemetry/sdk-trace-base` is planned for removal from the public API (ref).

This PR anticipates the possible breaking changes derived from it

Short description of the changes

  • Remove usage of new Span constructor in favour of Tracer.startSpan

@david-luna david-luna requested a review from a team as a code owner October 1, 2024 12:28
@github-actions github-actions bot added pkg:instrumentation-dns pkg-status:unmaintained This package is unmaintained. Only bugfixes may be acceped until a new owner has been found. labels Oct 1, 2024
@codecov Codecov
Copy link

codecov bot commented Oct 1, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.77%. Comparing base (dfb2dff) to head (21d2d0f).
Report is 248 commits behind head on main.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #2458      +/-   ##
==========================================
- Coverage   90.97%   90.77%   -0.21%     
==========================================
  Files         146      156      +10     
  Lines        7492     7716     +224     
  Branches     1502     1584      +82     
==========================================
+ Hits         6816     7004     +188     
- Misses        676      712      +36
Files with missing lines Coverage Δ
...ode/opentelemetry-instrumentation-dns/src/utils.ts 100.00% <100.00%> (ø)

... and 76 files with indirect coverage changes

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Oct 2, 2024

This package does not have an assigned component owner and is considered unmaintained. As such this package is in feature-freeze and this PR will be closed with 14 days unless a new owner or a sponsor (a member of @open-telemetry/javascript-approvers) for the feature is found. It is the responsibility of the author to find a sponsor for this feature.
Are you familiar with this package? Consider becoming a component owner.

MikeGoldsmith
MikeGoldsmith approved these changes Oct 2, 2024
View reviewed changes
Copy link
Member

@MikeGoldsmith MikeGoldsmith 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, thanks @david-luna. I've left one query regarding type coercion.

packages/baggage-span-processor/test/baggage-span-processor.test.ts Outdated
SpanKind.SERVER
);
ROOT_CONTEXT
) as unknown as Span;
Copy link
Member

Choose a reason for hiding this comment

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

Do we need both type coercions here - can we coerce straight into Span?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not necessary at all. Removing it in the next commit. Good catch :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed!

plugins/node/opentelemetry-instrumentation-dns/test/functionals/utils.test.ts Outdated
kind: SpanKind.INTERNAL,
},
ROOT_CONTEXT
) as unknown as Span;
Copy link
Member

Choose a reason for hiding this comment

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

Same as above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed!

@MikeGoldsmith
Copy link
Member

This package does not have an assigned component owner and is considered unmaintained. As such this package is in feature-freeze and this PR will be closed with 14 days unless a new owner or a sponsor (a member of @open-telemetry/javascript-approvers) for the feature is found. It is the responsibility of the author to find a sponsor for this feature. Are you familiar with this package? Consider becoming a component owner.

Looks like the warning will be for opentelemetry-instrumentation-dns, not baggage-span-procesor. We may need to break up the PR to address the different components due to their level of ownership / maintenance.

@pichlermarc
Copy link
Member

@MikeGoldsmith I'll sponsor the PR, then we don't need to break it up 🙂

@pichlermarc pichlermarc added has:sponsor This package or feature has a sponsor that has volunteered to review PRs and respond to questions and removed pkg-status:unmaintained:autoclose-scheduled labels Oct 2, 2024
@pichlermarc pichlermarc self-assigned this Oct 2, 2024
@github-actions github-actions bot requested a review from MikeGoldsmith October 2, 2024 11:28
david-luna
david-luna commented Oct 2, 2024
View reviewed changes
packages/baggage-span-processor/src/index.ts
export * from './baggage-span-processor';
export * from './types';
export { BaggageSpanProcessor } from './baggage-span-processor';
export { ALLOW_ALL_BAGGAGE_KEYS, BaggageKeyPredicate } from './types';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

note to reviewer: open-telemetry/opentelemetry-js#4186

@david-luna david-luna requested a review from pichlermarc October 2, 2024 11:29
pichlermarc
pichlermarc approved these changes Oct 2, 2024
View reviewed changes
Copy link
Member

@pichlermarc pichlermarc left a comment

Choose a reason for hiding this comment

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

Thanks @david-luna 🙂
approving for @opentelemetry/instrumentation-dns 🙂

MikeGoldsmith
MikeGoldsmith approved these changes Oct 2, 2024
View reviewed changes
Copy link
Member

@MikeGoldsmith MikeGoldsmith 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 me too - thanks @david-luna 🎉

@david-luna david-luna merged commit 37009ad into open-telemetry:main Oct 2, 2024
21 checks passed
@david-luna david-luna deleted the dluna/remove-new-span-usage branch October 2, 2024 13:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pichlermarc pichlermarc pichlermarc approved these changes

@MikeGoldsmith MikeGoldsmith MikeGoldsmith approved these changes

@JamieDanielson JamieDanielson Awaiting requested review from JamieDanielson

Assignees

@MikeGoldsmith MikeGoldsmith

@pichlermarc pichlermarc

@JamieDanielson JamieDanielson

Labels
has:sponsor This package or feature has a sponsor that has volunteered to review PRs and respond to questions pkg:instrumentation-dns pkg-status:unmaintained This package is unmaintained. Only bugfixes may be acceped until a new owner has been found.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@david-luna @MikeGoldsmith @pichlermarc @JamieDanielson