-
Notifications
You must be signed in to change notification settings - Fork 834
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
refactor(sdk-trace-base)!: remove Span class from exports #5048
refactor(sdk-trace-base)!: remove Span class from exports #5048
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## next #5048 +/- ##
=======================================
Coverage 93.24% 93.24%
=======================================
Files 318 318
Lines 8231 8231
Branches 1651 1651
=======================================
Hits 7675 7675
Misses 556 556
|
@@ -108,7 +108,6 @@ describe('NodeTracerProvider', () => { | |||
sampler: new AlwaysOnSampler(), | |||
}); | |||
const span = provider.getTracer('default').startSpan('my-span'); | |||
assert.ok(span instanceof Span); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note for reviewer: Span
is not a class anymore but a type. If necessary we could check that conforms to the api.Span
& ReadableSpan
interfaces.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's fine to leave it out, I think.
@pichlermarc sorry to bother. Is there a way to stop/relaunch the codecov check? seems to be stuck for a day now |
no worries, it looks like it actually kind of un-stuck itself just now. 🙂 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, thanks for working on this 👍
Which problem is this PR solving?
Removes the
Span
constructor from the exports so users are compelled to usetracer.startSpan
API which is safer. Instead of the class the SDK exports now a type with the same properties.Ref: #3597
Short description of the changes
Span
a type and not a classSpanImpl
SpanImpl
constructor internally (src & tests)Span
type inindex.ts
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Checklist:
main
branch intonext
([next] merge changes from main #5052)