-
-
Notifications
You must be signed in to change notification settings - Fork 4.5k
feat(issues): Include empty tags in group tagkey values endpoints #102048
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(issues): Include empty tags in group tagkey values endpoints #102048
Conversation
Add support for including empty tag values in the group tagkey values
endpoints when the organizations:issue-tags-include-empty-values feature
flag is enabled.
Endpoints affected:
- api/0/issues/{id}/tags/{key}/values/
- api/0/issues/{id}/tags/{key}/
Changes:
- Add include_empty_values parameter to get_group_tag_key, get_group_tag_value_iter,
get_group_tag_value_paginator, get_group_tag_value_count, and get_top_group_tag_values methods
- Update __get_tag_key_and_top_values to conditionally exclude empty values
- Update group_tagkey_values and group_tagkey_details endpoints to check feature flag
- Add feature flag organizations:issue-tags-include-empty-values
- Add tests for empty tag inclusion behavior
src/sentry/tagstore/snuba/backend.py
Outdated
| self.key_column: [key], | ||
| } | ||
| if not include_empty_values: | ||
| filters[self.key_column] = [key] |
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.
Bug: Tag Key Filter Ignored When Including Empty Values
When include_empty_values is True in get_group_tag_value_iter, the query incorrectly omits the filter for the specific tag key. This results in the method returning values for all tag keys in the group, instead of only the requested one. The include_empty_values parameter was intended to control filtering of empty values, not the tag key itself.
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.
Bot comment seems reasonable — can you explain why you're now omitting the key filter?
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.
added a comment
src/sentry/tagstore/snuba/backend.py
Outdated
| if not include_empty_values: | ||
| filters[self.key_column] = [key] | ||
| dataset, filters = self.apply_group_filters(group, filters) |
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.
Bug: When include_empty_values is true in get_group_tag_value_iter, the filter for the specific tag key is omitted, causing the query to return values for all tags.
Severity: CRITICAL | Confidence: 0.95
🔍 Detailed Analysis
In the `get_group_tag_value_iter` method, when `include_empty_values` is `True`, a filter to restrict the query to the specified tag `key` is not applied. The line `filters[self.key_column] = [key]` is only executed when `include_empty_values` is `False`. Without this filter, the Snuba query will fetch values from all tag keys for the given group, not just the one requested. This leads to incorrect data being returned to the client, including wrong tag values, inaccurate counts, and faulty pagination, as the aggregations and limits are applied to a much larger, incorrect dataset.
💡 Suggested Fix
The filter filters[self.key_column] = [key] should be applied regardless of the value of include_empty_values. Move this line of code so that it is executed before the if not include_empty_values: block, ensuring the query is always correctly filtered by the requested tag key.
🤖 Prompt for AI Agent
Fix this bug. In src/sentry/tagstore/snuba/backend.py at lines 1408-1410: In the
`get_group_tag_value_iter` method, when `include_empty_values` is `True`, a filter to
restrict the query to the specified tag `key` is not applied. The line
`filters[self.key_column] = [key]` is only executed when `include_empty_values` is
`False`. Without this filter, the Snuba query will fetch values from all tag keys for
the given group, not just the one requested. This leads to incorrect data being returned
to the client, including wrong tag values, inaccurate counts, and faulty pagination, as
the aggregations and limits are applied to a much larger, incorrect dataset.
Did we get this right? 👍 / 👎 to inform future reviews.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #102048 +/- ##
===========================================
+ Coverage 75.54% 80.90% +5.36%
===========================================
Files 8819 8822 +3
Lines 389819 389901 +82
Branches 24799 24799
===========================================
+ Hits 294475 315445 +20970
+ Misses 94982 74094 -20888
Partials 362 362 |
| # Whether to allow issue only search on the issue list | ||
| manager.add("organizations:issue-search-allow-postgres-only-search", OrganizationFeature, FeatureHandlerStrategy.FLAGPOLE, api_expose=False) | ||
| # Include empty tag count in issue tag totals/values | ||
| manager.add("organizations:issue-tags-include-empty-values", OrganizationFeature, FeatureHandlerStrategy.FLAGPOLE, api_expose=False) |
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.
This is in temporary.py — just confirming that we plan to roll this out to all orgs eventually?
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.
yes! asap. Felt dangerous enough to put behind a flag and it needs frontend changes
src/sentry/tagstore/snuba/backend.py
Outdated
| self.key_column: [key], | ||
| } | ||
| if not include_empty_values: | ||
| filters[self.key_column] = [key] |
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.
Bot comment seems reasonable — can you explain why you're now omitting the key filter?
thetruecpaul
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.
LGTM
…02048) Add support for including empty tag values in the group tagkey values endpoints when the organizations:issue-tags-include-empty-values feature flag is enabled. Currently will display an empty string and the count and will need to follow up with frontend fixes. <img width="924" height="341" alt="image" src="https://github.com/user-attachments/assets/abe4744e-ebbd-4340-bdaa-fe78f9af9957" /> part of https://linear.app/getsentry/issue/RTC-1181/
…02048) Add support for including empty tag values in the group tagkey values endpoints when the organizations:issue-tags-include-empty-values feature flag is enabled. Currently will display an empty string and the count and will need to follow up with frontend fixes. <img width="924" height="341" alt="image" src="https://github.com/user-attachments/assets/abe4744e-ebbd-4340-bdaa-fe78f9af9957" /> part of https://linear.app/getsentry/issue/RTC-1181/
Add support for including empty tag values in the group tagkey values endpoints when the organizations:issue-tags-include-empty-values feature flag is enabled.
Currently will display an empty string and the count and will need to follow up with frontend fixes.
part of https://linear.app/getsentry/issue/RTC-1181/