Skip to content

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

Merged
merged 2 commits into from
Jan 3, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
61 changes: 57 additions & 4 deletions packages/core/src/tracing/span.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@ import type {
Instrumenter,
Primitive,
Span as SpanInterface,
SpanAttributeValue,
SpanAttributes,
SpanContext,
SpanOrigin,
TraceContext,
Expand Down Expand Up @@ -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
*/
Expand Down Expand Up @@ -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';

Expand Down Expand Up @@ -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 {
Copy link
Member Author

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!

Copy link
Member

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.

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
*/
Expand Down Expand Up @@ -297,7 +320,7 @@ export class Span implements SpanInterface {
*/
public toContext(): SpanContext {
return dropUndefinedKeys({
data: this.data,
data: this._getData(),
Copy link
Member Author

Choose a reason for hiding this comment

The 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 undefined here? In contrast to toJSON() and getTraceContext()? I've aligned this here so that this always returns undefined if there is no data at all. cc @AbhiPrasad

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's fine that we never return undefined

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but is it also fine to return undefined? 😅 otherwise I can update this to data: this._getData() || {} but IMHO if we don't need this we rather avoid it?

Copy link
Member Author

Choose a reason for hiding this comment

The 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,
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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,
Expand All @@ -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 =
Expand Down
3 changes: 1 addition & 2 deletions packages/core/src/tracing/trace.ts
Original file line number Diff line number Diff line change
@@ -1,12 +1,11 @@
import type { TransactionContext } from '@sentry/types';
import type { Span, TransactionContext } from '@sentry/types';
import { dropUndefinedKeys, isThenable, logger, tracingContextFromHeaders } from '@sentry/utils';

import { DEBUG_BUILD } from '../debug-build';
import { getCurrentScope, withScope } from '../exports';
import type { Hub } from '../hub';
import { getCurrentHub } from '../hub';
import { hasTracingEnabled } from '../utils/hasTracingEnabled';
import type { Span } from './span';

/**
* Wraps a function with a transaction/span and finishes the span after the function is done.
Expand Down
158 changes: 158 additions & 0 deletions packages/core/test/lib/tracing/span.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -29,4 +29,162 @@ describe('span', () => {
expect(span.name).toEqual('new name');
expect(span.description).toEqual('new name');
});

describe('setAttribute', () => {
it('allows to set attributes', () => {
const span = new Span();

span.setAttribute('str', 'bar');
span.setAttribute('num', 1);
span.setAttribute('zero', 0);
span.setAttribute('bool', true);
span.setAttribute('false', false);
span.setAttribute('undefined', undefined);
span.setAttribute('numArray', [1, 2]);
span.setAttribute('strArray', ['aa', 'bb']);
span.setAttribute('boolArray', [true, false]);
span.setAttribute('arrayWithUndefined', [1, undefined, 2]);

expect(span.attributes).toEqual({
str: 'bar',
num: 1,
zero: 0,
bool: true,
false: false,
numArray: [1, 2],
strArray: ['aa', 'bb'],
boolArray: [true, false],
arrayWithUndefined: [1, undefined, 2],
});
});

it('deletes attributes when setting to `undefined`', () => {
const span = new Span();

span.setAttribute('str', 'bar');

expect(Object.keys(span.attributes).length).toEqual(1);

span.setAttribute('str', undefined);

expect(Object.keys(span.attributes).length).toEqual(0);
});

it('disallows invalid attribute types', () => {
const span = new Span();

/** @ts-expect-error this is invalid */
span.setAttribute('str', {});

/** @ts-expect-error this is invalid */
span.setAttribute('str', null);

/** @ts-expect-error this is invalid */
span.setAttribute('str', [1, 'a']);
});
});

describe('setAttributes', () => {
it('allows to set attributes', () => {
const span = new Span();

const initialAttributes = span.attributes;

expect(initialAttributes).toEqual({});

const newAttributes = {
str: 'bar',
num: 1,
zero: 0,
bool: true,
false: false,
undefined: undefined,
numArray: [1, 2],
strArray: ['aa', 'bb'],
boolArray: [true, false],
arrayWithUndefined: [1, undefined, 2],
};
span.setAttributes(newAttributes);

expect(span.attributes).toEqual({
str: 'bar',
num: 1,
zero: 0,
bool: true,
false: false,
numArray: [1, 2],
strArray: ['aa', 'bb'],
boolArray: [true, false],
arrayWithUndefined: [1, undefined, 2],
});

expect(span.attributes).not.toBe(newAttributes);

span.setAttributes({
num: 2,
numArray: [3, 4],
});

expect(span.attributes).toEqual({
str: 'bar',
num: 2,
zero: 0,
bool: true,
false: false,
numArray: [3, 4],
strArray: ['aa', 'bb'],
boolArray: [true, false],
arrayWithUndefined: [1, undefined, 2],
});
});

it('deletes attributes when setting to `undefined`', () => {
const span = new Span();

span.setAttribute('str', 'bar');

expect(Object.keys(span.attributes).length).toEqual(1);

span.setAttributes({ str: undefined });

expect(Object.keys(span.attributes).length).toEqual(0);
});
});

// Ensure that attributes & data are merged together
describe('_getData', () => {
it('works without data & attributes', () => {
const span = new Span();

expect(span['_getData']()).toEqual(undefined);
});

it('works with data only', () => {
const span = new Span();
span.setData('foo', 'bar');

expect(span['_getData']()).toEqual({ foo: 'bar' });
expect(span['_getData']()).toBe(span.data);
});

it('works with attributes only', () => {
const span = new Span();
span.setAttribute('foo', 'bar');

expect(span['_getData']()).toEqual({ foo: 'bar' });
expect(span['_getData']()).toBe(span.attributes);
});

it('merges data & attributes', () => {
const span = new Span();
span.setAttribute('foo', 'foo');
span.setAttribute('bar', 'bar');
span.setData('foo', 'foo2');
span.setData('baz', 'baz');

expect(span['_getData']()).toEqual({ foo: 'foo', bar: 'bar', baz: 'baz' });
expect(span['_getData']()).not.toBe(span.attributes);
expect(span['_getData']()).not.toBe(span.data);
});
});
});
2 changes: 1 addition & 1 deletion packages/node/src/integrations/undici/types.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
// Vendored from https://github.com/DefinitelyTyped/DefinitelyTyped/blob/5a94716c6788f654aea7999a5fc28f4f1e7c48ad/types/node/diagnostics_channel.d.ts

import type { URL } from 'url';
import type { Span } from '@sentry/core';
import type { Span } from '@sentry/types';

// License:
// This project is licensed under the MIT license.
Expand Down
4 changes: 2 additions & 2 deletions packages/sveltekit/src/server/handle.ts
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
/* eslint-disable @sentry-internal/sdk/no-optional-chaining */
import type { Span } from '@sentry/core';
import { getCurrentScope } from '@sentry/core';
import { getActiveTransaction, runWithAsyncContext, startSpan } from '@sentry/core';
import { captureException } from '@sentry/node';
/* eslint-disable @sentry-internal/sdk/no-optional-chaining */
import type { Span } from '@sentry/types';
import { dynamicSamplingContextToSentryBaggageHeader, objectify } from '@sentry/utils';
import type { Handle, ResolveOptions } from '@sveltejs/kit';

Expand Down
1 change: 0 additions & 1 deletion packages/tracing/test/span.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -480,7 +480,6 @@ describe('Span', () => {

expect(newContext).toStrictEqual({
...originalContext,
data: {},
spanId: expect.any(String),
startTimestamp: expect.any(Number),
tags: {},
Expand Down
2 changes: 1 addition & 1 deletion packages/types/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,7 @@ export type {

// eslint-disable-next-line deprecation/deprecation
export type { Severity, SeverityLevel } from './severity';
export type { Span, SpanContext, SpanOrigin } from './span';
export type { Span, SpanContext, SpanOrigin, SpanAttributeValue, SpanAttributes } from './span';
export type { StackFrame } from './stackframe';
export type { Stacktrace, StackParser, StackLineParser, StackLineParserFn } from './stacktrace';
export type { TextEncoderInternal } from './textencoder';
Expand Down
33 changes: 33 additions & 0 deletions packages/types/src/span.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
/**
Expand Down Expand Up @@ -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.
*/
Expand Down Expand Up @@ -118,6 +134,11 @@ export interface Span extends SpanContext {
*/
data: { [key: string]: any };

/**
* @inheritDoc
*/
attributes: SpanAttributes;
Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you mean the shape (SpanAttributes) or to have this at all on the span? This is also exposed this way on OTEL spans so I think there won't really be a way around exposing this for us as well 😅

Copy link
Contributor

Choose a reason for hiding this comment

The 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
*/
Expand Down Expand Up @@ -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
Expand Down
Loading