chore(conventions): Bump conventions to 0.21.0 - #6339
Conversation
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit be5ca7f. Configure here.
| #[metastructure(field = "http.decoded_response_content_length")] | ||
| pub http_decoded_response_content_length: Annotated<String>, | ||
| #[metastructure(field = "http.response_content_length")] | ||
| pub http_response_content_length: Annotated<String>, | ||
| #[metastructure(field = "http.response_transfer_size")] | ||
| pub http_response_transfer_size: Annotated<String>, |
There was a problem hiding this comment.
Just to make sure this is intended, everything here (in SentryTags) will be prefixed with sentry. in the attributes later on.
There was a problem hiding this comment.
Ahh, hrrm.
They don't have the sentry. prefix in conventions so i can just remove these?
There was a problem hiding this comment.
Chatted with @mjq, gonna revert my changes and leave as is
Dav1dde
left a comment
Dav1dde
left a comment
There was a problem hiding this comment.
Requesting changes until we figure out the deprecated on SentryTags. I think this is not correct.
| /// [span operations](https://develop.sentry.dev/sdk/performance/span-operations/) and | ||
| /// existing [span data](https://develop.sentry.dev/sdk/performance/span-data-conventions/) fields, | ||
| /// and rely on Sentry conventions and heuristics. | ||
| #[allow(deprecated)] |
There was a problem hiding this comment.
@loewenheim is this appropriate here? Pretty sure since we apply conventions to span data now, this must actually use the latest version of the convention attributes and the deprecations must be addressed here. Is that right?
There was a problem hiding this comment.
I'm not sure how to address this, we do not want a sentry. prefix on these new attributes, so from my understanding they cannot be put into SentryTags.
There was a problem hiding this comment.
You need to remove the allow(deprecated) then follow the deprecation warnings to resolve them. An attribute was deprecated and replaced with a new one, you need to now use the new one instead of the old renamed/deprecated one.
There was a problem hiding this comment.
@Dav1dde I don't think that's quite right - tbh I don't think these should be using sentry conventions constants at all. As you pointed out they all get prefixed with sentry. before being materialized to attributes - the prefixed versions are what actually live in conventions, if they're conventional at all. The fact that we're using the same string name here for a sentry tag as an existing attribute name constant is actually a coincidence - tbh the better fix might be to stop using the constants at all because it's misleading. At this point in the code, they aren't attribute names, nor will they be copied unmodified into attribute names downstream.
There was a problem hiding this comment.
It's not a coincidence. The tags are filled from SpanData, which is migrated based on conventions. If SentryTags is then filled from a deprecated attribute it will always pick the wrong one (with normalize it will never have a value to pick).
There was a problem hiding this comment.
@Dav1dde Ah. Thank you for explaining. Huh. In this case the read side and write side are now going to disagree, right? Just because the read side name (in SpanData) has been deprecated, that has no impact on the expected write side name (sentry.-prefixed attribute). Right? If there are any dependencies downstream in buffer, Sentry API, etc they would be on the existing written, sentry.-prefixed version, regardless of how the incoming data changes. Or am I still confused?
There was a problem hiding this comment.
I think it sounds like we should start killing the SentryTags and its sentry. prefixed counterparts, at least for everything which already has a non sentry. counterpart.
Although I don't know what will break if we start doing that (existing queries on sentry. with missing aliases).
There was a problem hiding this comment.
I think if we just update the reads and don't change the attributes SentryTags will materialize into, then nothing should break (except maybe someone's brain if they have to make sense of this in a year from now).
Dav1dde
left a comment
Dav1dde
left a comment
There was a problem hiding this comment.
oops misclicked before, see previous review
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
|
Maybe makes sense to wait for @mjq to give it another look, from my pov it looks good. |
mjq
left a comment
mjq
left a comment
There was a problem hiding this comment.
LGTM, but I'd delete that orphaned snapshot file for clarity 🙏 Thanks!
| http_response_transfer_size: ~, | ||
| http_response_body_decoded_size: ~, | ||
| http_response_body_size: ~, | ||
| http_response_size: ~, |
There was a problem hiding this comment.
@nsdeschenes this test actually doesn't exist anymore which is why things pass even with this change - you can/should delete the whole file ✂️
3 participants
Bumping conventions to the latest version
0.21.0