feat(attributes): Ensure replacements aren't deprecated#391
Merged
Conversation
When a field is deprecated, it should point to a non-deprecated replacement. This should obviously be true for newly introduced deprecations, but is a tradeoff for the case where: - field A is deprecated and replaced by B - later, field B is deprecated and replaced by C This would now cause a test failure, and A would need to be updated to point to C instead of B. However, on the plus side: - much easier to programatically translate or replace fields with their latest replacement, since you don't have to follow a chain of attribute definitions - avoids the trap where a human sees a deprecated field and replaces it with the advertised replacement, not realizing that it's _also_ deprecated I think on the whole this is worth it. --- 🤖: Claude (Opus 4.6) used to write the test; validated by hand. All words my own.
Semver Impact of This PR🟡 Minor (new features) 📋 Changelog PreviewThis is how your changes will appear in the changelog. New Features ✨Attributes
Other
Bug Fixes 🐛Attributes
Other
Internal Changes 🔧Deps
Other
🤖 This preview updates automatically when you update the PR. |
mjq
changed the title
mjq
changed the title
nsdeschenes
approved these changes
May 20, 2026
Lms24
approved these changes
May 20, 2026
Member
Lms24
left a comment
Lms24
left a comment
There was a problem hiding this comment.
good new test, thanks! FYI @alexander-alderman-webb for gen_ai attributes
4 tasks
mjq
added a commit
to getsentry/snuba
that referenced
this pull request
May 21, 2026
Currently when EAP is queried via RPC for a particular attribute name, the underlying query will coalesce that attribute name with any of its deprecated aliases. For example, given an attribute `A` that was deprecated in favour of `B`, that was then deprecated in favour of `C`, querying for `C` will in fact resolve to `coalesce(C, B, A)`. This helps keeps queries working despite attribute deprecations. _(Actually, the ordering of `B, A` is currently non-deterministic, but this PR addresses that.)_ **However**, this requires that the caller always use the latest name for an attribute - for example, a query for `A` would only search `A`, and a query for `B` would only search `B`. Only `C` triggers the coalescing. Our desired behaviour is that a search for any of `A`, `B`, or `C` triggers a coalesced query on all aliases. There are two possible approaches to take: 1. All RPC callers must use the `sentry-conventions` library to replace attribute names with their latest non-deprecated alias before calling EAP, or get inconsistent results. 2. Snuba always performs coalescing, for deprecated as well as non-deprecated attributes. Option 1 is challenging because of how Sentry's Alerts work, which store RPC calls. Even if these calls use the latest attribute at creation time, a later deprecation coming in would cause incorrect result until they were recreated, and we would need to create a mechanism to do so whenever Snuba's `sentry-conventions` library was updated. To avoid duplicating the deprecated attribute logic in every caller and dealing with this conventions/alerts coupling, I'm suggesting option 2, which this PR implements. Note the subtleties around **coalesce ordering**: - The requested attribute is always first. This is the most consistent option for end users and gives callers a bit of control over behaviour for edge cases. - If the requested attribute is deprecated, then its non-deprecated alias is always second. We wouldn't want a deprecated alias taking precedence over the current, non-deprecated one. - Finally, all remaining deprecated aliases are listed in alphabetically sorted order. This ordering is arbitrary because we don't store metadata around the order in which fields are deprecated, but enforcing an ordering is nonetheless important so that we don't get non-deterministic results. This PR also updates `sentry-conventions` to the latest version in order to get getsentry/sentry-conventions#391 (ensures that all deprecated attributes point to a non-deprecated replacement; ensures that `_resolve_canonical` only ever has to make 1 hop). --- 🤖: Claude Code (Opus 4.6) used to generate the code, with human editing + review. All words my own.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
When an attribute is deprecated, it should point to a non-deprecated replacement.
This should obviously be true for newly introduced deprecations, but is a tradeoff for the case where:
This would now cause a test failure, and A would need to also be updated to point to C instead of B.
However, on the plus side:
I think on the whole this is worth it.
🤖: Claude (Opus 4.6) used to write the test; validated by hand. All words my own.