feat(core): Add spanGetMetadata() & spanSetMetadata() APIs
#10041
Conversation
size-limit report 📦
|
mydea
force-pushed
the
fn/span-metadata
branch
3 times, most recently
from
ae86c48
to
4f1bd80
Compare
mydea
force-pushed
the
fn/span-metadata
branch
from
4f1bd80
to
9cb7f1c
Compare
There was a problem hiding this comment.
2 questions:
- should we change the naming scheme to
(get|set)SpanMetadata? I guess you want to mimic the span.getMetadata name but it seems a bit unintuitive to read in a post-v7 world if that makes sense. - do we still need the getter after feat(core): Add
spanToJSON()method to get span properties #10074?
To replace `transaction.setMetadata()`.
mydea
force-pushed
the
fn/span-metadata
branch
from
cb834d1
to
d34f47d
Compare
Re: naming: No strong opinions 🤔 My thought was to make auto-complete a bit easier by typing Re: your second point: I think not, as the metadata is not part of the span serialization. IMHO it's ok to have this separate, as it's basically a way to store arbitrary data that needs to be attached to a span for us internally during serialization only 🤔 |
I just think we should be consistent and names should make sense after we move away from v7 and forget that this function was a replacement for a span method. If we already have
Ah good point, didn't think about this. |
…0097) This deprecates any usage of `metadata` on transactions. The main usages we have are to set `sampleRate` and `source` in there. These I replaced with semantic attributes. For backwards compatibility, when creating the transaction event we still check the metadata there as well. Other usage of metadata (mostly around `request`) remains intact for now, we need to replace this in v8 - e.g. put this on the isolation scope, probably. This is the first usage of [Semantic Attributes](https://github.com/getsentry/rfcs/blob/main/text/0116-sentry-semantic-conventions.md) in the SDK! This replaces #10041
To replace
transaction.setMetadata().I decided to make the metadata shapeless, so you can theoretically store anything there - but are also required to guard when reading this. Other than this, it feels a bit weird to store the transaction-related data we need right now there... 🤔