-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
feat(core): Add attributes
to Span
#10008
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
Changes from all commits
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 |
---|---|---|
|
@@ -3,6 +3,8 @@ import type { | |
Instrumenter, | ||
Primitive, | ||
Span as SpanInterface, | ||
SpanAttributeValue, | ||
SpanAttributes, | ||
SpanContext, | ||
SpanOrigin, | ||
TraceContext, | ||
|
@@ -104,6 +106,11 @@ export class Span implements SpanInterface { | |
// eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
public data: { [key: string]: any }; | ||
|
||
/** | ||
* @inheritDoc | ||
*/ | ||
public attributes: SpanAttributes; | ||
|
||
/** | ||
* List of spans that were finalized | ||
*/ | ||
|
@@ -137,6 +144,7 @@ export class Span implements SpanInterface { | |
this.startTimestamp = spanContext.startTimestamp || timestampInSeconds(); | ||
this.tags = spanContext.tags || {}; | ||
this.data = spanContext.data || {}; | ||
this.attributes = spanContext.attributes || {}; | ||
this.instrumenter = spanContext.instrumenter || 'sentry'; | ||
this.origin = spanContext.origin || 'manual'; | ||
|
||
|
@@ -217,12 +225,27 @@ export class Span implements SpanInterface { | |
/** | ||
* @inheritDoc | ||
*/ | ||
// eslint-disable-next-line @typescript-eslint/no-explicit-any, @typescript-eslint/explicit-module-boundary-types | ||
// eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
public setData(key: string, value: any): this { | ||
this.data = { ...this.data, [key]: value }; | ||
return this; | ||
} | ||
|
||
/** @inheritdoc */ | ||
public setAttribute(key: string, value: SpanAttributeValue | undefined): void { | ||
if (value === undefined) { | ||
// eslint-disable-next-line @typescript-eslint/no-dynamic-delete | ||
delete this.attributes[key]; | ||
} else { | ||
this.attributes[key] = value; | ||
} | ||
} | ||
|
||
/** @inheritdoc */ | ||
public setAttributes(attributes: SpanAttributes): void { | ||
Object.keys(attributes).forEach(key => this.setAttribute(key, attributes[key])); | ||
} | ||
|
||
/** | ||
* @inheritDoc | ||
*/ | ||
|
@@ -297,7 +320,7 @@ export class Span implements SpanInterface { | |
*/ | ||
public toContext(): SpanContext { | ||
return dropUndefinedKeys({ | ||
data: this.data, | ||
data: this._getData(), | ||
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. Just to double check, I guess/hope it is not important that we never return 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 think it's fine that we never return 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. but is it also fine to return 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. We'll I'm gonna merge this anyway, I think it should be fine, otherwise we can adjust it later. |
||
description: this.description, | ||
endTimestamp: this.endTimestamp, | ||
op: this.op, | ||
|
@@ -335,7 +358,7 @@ export class Span implements SpanInterface { | |
*/ | ||
public getTraceContext(): TraceContext { | ||
return dropUndefinedKeys({ | ||
data: Object.keys(this.data).length > 0 ? this.data : undefined, | ||
data: this._getData(), | ||
description: this.description, | ||
op: this.op, | ||
parent_span_id: this.parentSpanId, | ||
|
@@ -365,7 +388,7 @@ export class Span implements SpanInterface { | |
origin?: SpanOrigin; | ||
} { | ||
return dropUndefinedKeys({ | ||
data: Object.keys(this.data).length > 0 ? this.data : undefined, | ||
data: this._getData(), | ||
description: this.description, | ||
op: this.op, | ||
parent_span_id: this.parentSpanId, | ||
|
@@ -378,6 +401,36 @@ export class Span implements SpanInterface { | |
origin: this.origin, | ||
}); | ||
} | ||
|
||
/** | ||
* Get the merged data for this span. | ||
* For now, this combines `data` and `attributes` together, | ||
* until eventually we can ingest `attributes` directly. | ||
*/ | ||
private _getData(): | ||
| { | ||
// eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
[key: string]: any; | ||
} | ||
| undefined { | ||
const { data, attributes } = this; | ||
|
||
const hasData = Object.keys(data).length > 0; | ||
const hasAttributes = Object.keys(attributes).length > 0; | ||
|
||
if (!hasData && !hasAttributes) { | ||
return undefined; | ||
} | ||
|
||
if (hasData && hasAttributes) { | ||
return { | ||
...data, | ||
...attributes, | ||
}; | ||
} | ||
|
||
return hasData ? data : attributes; | ||
} | ||
} | ||
|
||
export type SpanStatusType = | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -12,6 +12,17 @@ export type SpanOrigin = | |
| `${SpanOriginType}.${SpanOriginCategory}.${SpanOriginIntegrationName}` | ||
| `${SpanOriginType}.${SpanOriginCategory}.${SpanOriginIntegrationName}.${SpanOriginIntegrationPart}`; | ||
|
||
// These types are aligned with OpenTelemetry Span Attributes | ||
export type SpanAttributeValue = | ||
| string | ||
| number | ||
| boolean | ||
| Array<null | undefined | string> | ||
| Array<null | undefined | number> | ||
| Array<null | undefined | boolean>; | ||
|
||
export type SpanAttributes = Record<string, SpanAttributeValue | undefined>; | ||
|
||
/** Interface holding all properties that can be set on a Span on creation. */ | ||
export interface SpanContext { | ||
/** | ||
|
@@ -65,6 +76,11 @@ export interface SpanContext { | |
*/ | ||
data?: { [key: string]: any }; | ||
|
||
/** | ||
* Attributes of the Span. | ||
*/ | ||
attributes?: SpanAttributes; | ||
|
||
/** | ||
* Timestamp in seconds (epoch time) indicating when the span started. | ||
*/ | ||
|
@@ -118,6 +134,11 @@ export interface Span extends SpanContext { | |
*/ | ||
data: { [key: string]: any }; | ||
|
||
/** | ||
* @inheritDoc | ||
*/ | ||
attributes: SpanAttributes; | ||
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. Maybe let's mark this as experimental in case we want to change something before v8. 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. you mean the shape ( 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. ah okay, I didn't know that. If we know it's gonna stick around it's fine 👍 |
||
|
||
/** | ||
* The transaction containing this span | ||
*/ | ||
|
@@ -156,6 +177,18 @@ export interface Span extends SpanContext { | |
*/ | ||
setData(key: string, value: any): this; | ||
|
||
/** | ||
* Set a single attribute on the span. | ||
* Set it to `undefined` to remove the attribute. | ||
*/ | ||
setAttribute(key: string, value: SpanAttributeValue | undefined): void; | ||
|
||
/** | ||
* Set multiple attributes on the span. | ||
* Any attribute set to `undefined` will be removed. | ||
*/ | ||
setAttributes(attributes: SpanAttributes): void; | ||
|
||
/** | ||
* Sets the status attribute on the current span | ||
* See: {@sentry/tracing SpanStatus} for possible values | ||
|
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 chose to implement this in a mutating way, for a slight performance improvment I guess. But no strong feelings, if we prefer to always clone the attributes (like we currently do for data etc) we can also do that!
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 I'd argue that this implementation (be it mutable or not) is more correct since we're deleting the property when passing
undefined
. Should be fine.