-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
feat(core): Align Span interface with OTEL #12898
Conversation
size-limit report 📦
|
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.
Do we want to go through despite the breaking change?
@@ -118,18 +151,21 @@ export class SentrySpan implements Span { | |||
} | |||
|
|||
/** @inheritdoc */ | |||
public setAttribute(key: string, value: SpanAttributeValue | undefined): void { | |||
public setAttribute(key: string, value: SpanAttributeValue | undefined): this { |
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.
Technically that's a breaking change 😬
Sure, 99% of our users will simply call span.setAttribute
but maybe for some reason someone asserts on undefined
being returned. Is there a concrete reason (e.g. someone running into this mismatch) we should consider breaking here? If not, I'd vote we don't change the return types of setAttribute(s)
in v8 but happy to discuss this.
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.
This is not breaking unless somebody implements the SentrySpan interface, which is not really something we recommend anyone doing.
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.
Hmm maybe I'm missing something but this (including variations of working with the return type) seems like valid code to me:
Sentry.startSpan({}, span => {
assert(span.setAttribute(...) === undefined)
})
I'm not saying this is a likely code path. We can make the call to technically break here. If we do, I'd prefer to extract the breaking aspect into a separate PR to revert more easily in case someone does write in.
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.
Widening return types is generally not considered semver major, but sever minor I believe.
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.
I'd say this is fine, we generally accept widening return types as non-breaking I believe, the potential for breaking here is extremely low 🤔
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.
Ok, I'll keep the change then?
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.
Yeah, let's keep it.
* @hidden | ||
* @internal | ||
*/ | ||
public recordException(_exception: unknown, _time?: number | undefined): void { |
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.
comment/unactionable: "recordException" is a very unfortunate function name for users seeing this method when interacting with the Sentry SDK 😬
(ignore this comment, I'm just fairly sure someone is gonna call it "accidentally" instead of captureException
)
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.
Had the same thought! I had an idea that we could alias this to captureException. Maybe we should also make it very clear in the JS doc that captureException needs to be preferred.
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.
I had an idea that we could alias this to captureException
yeah this also crossed my mind. But I'd rather do it when it comes up to save on bundle size.
@@ -118,18 +151,21 @@ export class SentrySpan implements Span { | |||
} | |||
|
|||
/** @inheritdoc */ | |||
public setAttribute(key: string, value: SpanAttributeValue | undefined): void { | |||
public setAttribute(key: string, value: SpanAttributeValue | undefined): this { |
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.
Yeah, let's keep it.
fixes #12566