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

feat(core): Align Span interface with OTEL #12898

Merged
merged 5 commits into from
Jul 18, 2024
Merged

feat(core): Align Span interface with OTEL #12898

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
7cd2677
feat: align Span interface with otel
chargome Jul 12, 2024
0440b74
Merge branch 'develop' into cg/align-span-interface
chargome Jul 12, 2024
c2d6b90
Merge branch 'develop' into cg/align-span-interface
chargome Jul 15, 2024
93c7486
fix: rephrase comment
chargome Jul 17, 2024
832c767
Merge branch 'develop' into cg/align-span-interface
chargome Jul 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 33 additions & 0 deletions packages/core/src/tracing/sentryNonRecordingSpan.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,4 +68,37 @@ export class SentryNonRecordingSpan implements Span {
): this {
return this;
}

/**
* This should generally not be used,
* but we need it for being comliant with the OTEL Span interface.
*
* @hidden
* @internal
*/
public addLink(_link: unknown): this {
return this;
}

/**
* This should generally not be used,
* but we need it for being comliant with the OTEL Span interface.
*
* @hidden
* @internal
*/
public addLinks(_links: unknown[]): this {
return this;
}

/**
* This should generally not be used,
* but we need it for being comliant with the OTEL Span interface.
*
* @hidden
* @internal
*/
public recordException(_exception: unknown, _time?: number | undefined): void {
// noop
}
}
40 changes: 38 additions & 2 deletions packages/core/src/tracing/sentrySpan.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,39 @@ export class SentrySpan implements Span {
}
}

/**
* This should generally not be used,
* but we need it for being comliant with the OTEL Span interface.
chargome marked this conversation as resolved.
Show resolved Hide resolved
*
* @hidden
* @internal
*/
public addLink(_link: unknown): this {
return this;
}

/**
* This should generally not be used,
* but we need it for being comliant with the OTEL Span interface.
*
* @hidden
* @internal
*/
public addLinks(_links: unknown[]): this {
return this;
}

/**
* This should generally not be used,
* but we need it for being comliant with the OTEL Span interface.
*
* @hidden
* @internal
*/
public recordException(_exception: unknown, _time?: number | undefined): void {
Copy link
Member

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)

Copy link
Member

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.

Copy link
Member

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.

// noop
}

/** @inheritdoc */
public spanContext(): SpanContextData {
const { _spanId: spanId, _traceId: traceId, _sampled: sampled } = this;
Expand All @@ -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 {
Copy link
Member

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.

Copy link
Member

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.

Copy link
Member

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.

Copy link
Member

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.

Copy link
Member

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 🤔

Copy link
Member Author

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?

Copy link
Member

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.

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;
}

/**
Expand Down
4 changes: 1 addition & 3 deletions packages/opentelemetry/src/trace.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -289,9 +289,7 @@ export function continueTrace<T>(options: Parameters<typeof baseContinueTrace>[0
function getActiveSpanWrapper<T>(parentSpan?: Span | SentrySpan): (callback: () => T) => T {
return parentSpan
? (callback: () => T) => {
// We cast this, because the OTEL Span has a few more methods than our Span interface
// TODO: Add these missing methods to the Span interface
return withActiveSpan(parentSpan as Span, callback);
return withActiveSpan(parentSpan, callback);
}
: (callback: () => T) => callback();
}
19 changes: 17 additions & 2 deletions packages/types/src/span.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -219,13 +219,13 @@ export interface Span {
* Set a single attribute on the span.
* Set it to `undefined` to remove the attribute.
*/
setAttribute(key: string, value: SpanAttributeValue | undefined): void;
setAttribute(key: string, value: SpanAttributeValue | undefined): this;

/**
* Set multiple attributes on the span.
* Any attribute set to `undefined` will be removed.
*/
setAttributes(attributes: SpanAttributes): void;
setAttributes(attributes: SpanAttributes): this;

/**
* Sets the status attribute on the current span.
Expand All @@ -247,4 +247,19 @@ export interface Span {
* Adds an event to the Span.
*/
addEvent(name: string, attributesOrStartTime?: SpanAttributes | SpanTimeInput, startTime?: SpanTimeInput): this;

/**
* NOT USED IN SENTRY, only added for compliance with OTEL Span interface
*/
addLink(link: unknown): this;

/**
* NOT USED IN SENTRY, only added for compliance with OTEL Span interface
*/
addLinks(links: unknown): this;

/**
* NOT USED IN SENTRY, only added for compliance with OTEL Span interface
*/
recordException(exception: unknown, time?: number): void;
}
Loading