-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Changes from all commits
7cd2677
0440b74
c2d6b90
93c7486
832c767
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -107,6 +107,39 @@ export class SentrySpan implements Span { | |
} | ||
} | ||
|
||
/** | ||
* This should generally not be used, | ||
* but it is needed for being compliant with the OTEL Span interface. | ||
* | ||
* @hidden | ||
* @internal | ||
*/ | ||
public addLink(_link: unknown): this { | ||
return this; | ||
} | ||
|
||
/** | ||
* This should generally not be used, | ||
* but it is needed for being compliant with the OTEL Span interface. | ||
* | ||
* @hidden | ||
* @internal | ||
*/ | ||
public addLinks(_links: unknown[]): this { | ||
return this; | ||
} | ||
|
||
/** | ||
* This should generally not be used, | ||
* but it is needed for being compliant with the OTEL Span interface. | ||
* | ||
* @hidden | ||
* @internal | ||
*/ | ||
public recordException(_exception: unknown, _time?: number | undefined): void { | ||
// noop | ||
} | ||
|
||
/** @inheritdoc */ | ||
public spanContext(): SpanContextData { | ||
const { _spanId: spanId, _traceId: traceId, _sampled: sampled } = this; | ||
|
@@ -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 commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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 commentThe 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 commentThe 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 commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, let's keep it. |
||
if (value === undefined) { | ||
// eslint-disable-next-line @typescript-eslint/no-dynamic-delete | ||
delete this._attributes[key]; | ||
} else { | ||
this._attributes[key] = value; | ||
} | ||
|
||
return this; | ||
} | ||
|
||
/** @inheritdoc */ | ||
public setAttributes(attributes: SpanAttributes): void { | ||
public setAttributes(attributes: SpanAttributes): this { | ||
Object.keys(attributes).forEach(key => this.setAttribute(key, attributes[key])); | ||
return 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.
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.
yeah this also crossed my mind. But I'd rather do it when it comes up to save on bundle size.