-
-
Notifications
You must be signed in to change notification settings - Fork 4.5k
feat(issues): Include empty tags in group tags endpoint #102049
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
Conversation
Add support for including empty tag values in the group tags endpoint
(api/0/issues/{id}/tags/) when the organizations:issue-tags-include-empty-values
feature flag is enabled.
Changes:
- Add include_empty_values parameter to get_group_tag_keys_and_top_values methods
- Add __get_empty_value_stats_map helper method to query empty tag stats
- Update group_tags endpoint to check feature flag and pass parameter
- Add feature flag organizations:issue-tags-include-empty-values
- Add tests for empty tag inclusion behavior
|
|
||
| include_empty_values = features.has( | ||
| "organizations:issue-tags-include-empty-values", | ||
| organization=group.project.organization, |
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.
if we're doing this on group.project.org, would making this a project option be better?
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 need it behind a flag, org is usually the easier one to roll out
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #102049 +/- ##
===========================================
+ Coverage 80.95% 80.97% +0.02%
===========================================
Files 8797 8763 -34
Lines 389822 389456 -366
Branches 24800 24740 -60
===========================================
- Hits 315588 315373 -215
+ Misses 73860 73726 -134
+ Partials 374 357 -17 |
| # 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.
you mentioned you'll need this in the frontend, will you want api_expose=True?
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 think in the frontend i can just handle empty strings and let the backend decide when to send them the value is "" which doesn't exist today.
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 think this looks fine, and it makes sense to me, but i'd wait for an approval from someone more familiar with this endpoint
| if not stats or stats.get("count", 0) <= 0: | ||
| continue | ||
| vals = values_by_key.get(k, {}) |
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.
nit: from my understanding think these need to be .get(), since empty_stats_map is either {} (so the iteration won't run) or are guarenteed to have k (same with above for count)?
| response = self.client.get(url, format="json") | ||
| assert response.status_code == 200 | ||
| values = {v["value"] for v in response.data[0]["topValues"]} | ||
| assert values == {"", "bar"} |
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.
can we also check that the empty tag count is correct (so response.data[0]["topValues"]["count"] is 1 or whatever)
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'm kind of confused about why we want to allow this, what's the value of a tag with no name?
If a customer wants some dummy value, they can just write a tag with DUMMY as the name. Tbh, I don't think we should do this, especially since it causes an extra query
| empty_results = snuba.query( | ||
| dataset=dataset, | ||
| start=start, | ||
| end=end, | ||
| groupby=None, | ||
| conditions=conditions, | ||
| filter_keys=empty_filters, | ||
| aggregations=[], | ||
| selected_columns=selected_columns_empty, | ||
| referrer="tagstore._get_tag_keys_and_top_values_empty_counts", | ||
| tenant_ids=tenant_ids, | ||
| ) |
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.
Are we going to end up making an extra query for every org to fetch these empty keys?
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.
Ok, I read through the linear and I understand what we're trying to do here... I'm going to think a little on the query and see if we can avoid a second one
|
|
||
| # Then supplement the key objects with the top values for each. | ||
| if include_empty_values: | ||
| keys_to_check = list(values_by_key.keys()) or [k.key for k in keys_with_counts] |
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 Logic Fails with Empty Values
The keys_to_check logic in get_group_tag_keys_and_top_values incorrectly determines which keys to check for empty values. The expression list(values_by_key.keys()) or [k.key for k in keys_with_counts] can omit tags that only have empty values, especially when other tags have non-empty values or when specific keys are requested. This leads to incomplete tag data.
In #102049 added the empty tags, but noticing the total values no longer makes sense. Increases the total count when empty values is active
In #102049 added the empty tags, but noticing the total values no longer makes sense. Increases the total count when empty values is active part of https://linear.app/getsentry/issue/RTC-1181/
Issues attributed to commits in this pull requestThis pull request was merged and Sentry observed the following issues: |
Add support for including empty tag values in the group tags endpoint
(api/0/issues/{id}/tags/) when the
organizations:issue-tags-include-empty-values feature flag is enabled.
Makes a 2nd snuba query and attaches the empty values. I couldn't figure
out how to make this work in a single snuba query, but i did figure out
how to do this without N snuba queries.
also needs some follow up frontend work
<img width="454" height="256" alt="image"
src="https://github.com/user-attachments/assets/6b935125-7ed5-4418-bcbb-f4bbc7349990"
/>
part of https://linear.app/getsentry/issue/RTC-1181/
In #102049 added the empty tags, but noticing the total values no longer makes sense. Increases the total count when empty values is active part of https://linear.app/getsentry/issue/RTC-1181/
Add support for including empty tag values in the group tags endpoint (api/0/issues/{id}/tags/) when the organizations:issue-tags-include-empty-values feature flag is enabled.
Makes a 2nd snuba query and attaches the empty values. I couldn't figure out how to make this work in a single snuba query, but i did figure out how to do this without N snuba queries.
also needs some follow up frontend work
part of https://linear.app/getsentry/issue/RTC-1181/