-
Notifications
You must be signed in to change notification settings - Fork 106
feat(spanv2): Double write to legacy attributes for backwards compatibility #5490
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
feat(spanv2): Double write to legacy attributes for backwards compatibility #5490
Conversation
24e46c9 to
a2d9fd9
Compare
d9ac0b2 to
3f7dd38
Compare
180b946 to
79e7eea
Compare
79e7eea to
acfecc8
Compare
|
@loewenheim could this be covered by generic double-write rules in sentry conventions instead? |
Dav1dde
left a comment
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.
What @jjbayer mentioned, would be interested if the deprecation flow we added to conventions doesn't already cover this case, as it seems quite straight forward 1:1 mapping. If it does not, what changes do we need to make to get this covered?
| // This is equivalent to conditionally scrubbing by span category in the V1 pipeline. | ||
| if !attributes.contains_key(HTTP_REQUEST_METHOD) { | ||
| if !attributes.contains_key(HTTP_REQUEST_METHOD) | ||
| && !attributes.contains_key("http.request_method") |
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.
We want to have all attributes used defined in relay-conventions/src/consts.rs and added to conventions, unless there is a good reason not to add it to conventions.
I think even adding (still in use) legacy attributes to the conventions has 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 just thought that since legacy attributes like http.request_method don't align with OTel and we're going in a direction where the SDKs won't be sending it at all, it would be counter productive to include it. I do also see value in adding it to conventions tho because that attribute will always be present in some spans (even with span first, there will be people with unupgraded SDKs I guess).
The problem is that the mapping isn't 1:1, for example So to make this work we would probably have to add something like a legacy/fallback field to conventions where you can specify fields to write back to. That way, we don't have to falsely mark deprecation, and we would know which attribute |
|
Following up on this, I've added the attributes to sentry conventions (PR) and updated my changes accordingly. Note: I wasn't able to add |
Double writes SpanV2 attributes to legacy attributes to achieve backwards compatibility. With both conventions present in a span, V2 spans will continue to be compatible with our products while we simultaneously start accumulating spans that conform to sentry conventions.
DB and HTTP spans both fall back to legacy attributes, this is done so that when the transactions span pipeline gets unified into the span first pipeline, they continue to be normalized.
See DACI for more information: https://www.notion.so/sentry/DACI-Backwards-compatibility-for-span-first-attribute-conventions-2c48b10e4b5d802dbfc3d90610b2537b?source=copy_link
Note: bumps sentry conventions