feat(core): Add attributes to Span
#10008
Conversation
| public setData(key: string, value: any): this { | ||
| this.data = { ...this.data, [key]: value }; | ||
| return this; | ||
| } | ||
|
|
||
| /** @inheritdoc */ | ||
| public setAttribute(key: string, value: SpanAttributeValue | undefined): void { |
There was a problem hiding this comment.
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.
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.
size-limit report 📦
|
| public setData(key: string, value: any): this { | ||
| this.data = { ...this.data, [key]: value }; | ||
| return this; | ||
| } | ||
|
|
||
| /** @inheritdoc */ | ||
| public setAttribute(key: string, value: SpanAttributeValue | undefined): void { |
There was a problem hiding this comment.
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.
| /** | ||
| * @inheritDoc | ||
| */ | ||
| attributes: SpanAttributes; |
There was a problem hiding this comment.
Maybe let's mark this as experimental in case we want to change something before v8.
There was a problem hiding this comment.
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 😅
There was a problem hiding this comment.
ah okay, I didn't know that. If we know it's gonna stick around it's fine 👍
Together with `setAttribute()` and `setAttributes()` APIs, mirroring the OpenTelemetry API for their spans. For now, these are stored as `data` on spans/transactions, until we "directly" support them.
| @@ -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.
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
There was a problem hiding this comment.
I think it's fine that we never return undefined
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
We'll I'm gonna merge this anyway, I think it should be fine, otherwise we can adjust it later.
mydea
force-pushed
the
fn/span-attributes
branch
from
d6cbd39
to
641d75d
Compare
Together with `setAttribute()` and `setAttributes()` APIs, mirroring the OpenTelemetry API for their spans. For now, these are stored as `data` on spans/transactions, until we "directly" support them.
Together with
setAttribute()andsetAttributes()APIs, mirroring the OpenTelemetry API for their spans.For now, these are stored as
dataon spans/transactions, until we "directly" support them.