fix(eap): Map virtual context columns properly for topn queries #90343
Conversation
github-actions
bot
added
the
Scope: Backend
| @@ -2568,6 +2568,32 @@ def test_device_class_filter_unknown(self): | |||
| assert data[0]["device.class"] == "Unknown" | |||
| assert meta["dataset"] == self.dataset | |||
|
|
|||
| def test_device_class_column(self): | |||
There was a problem hiding this comment.
extra test to make sure i didn't break events/
Codecov ReportAttention: Patch coverage is ✅ All tests successful. No failed tests found.
Additional details and impacted files@@ Coverage Diff @@
## master #90343 +/- ##
==========================================
+ Coverage 87.81% 87.82% +0.01%
==========================================
Files 10275 10258 -17
Lines 579435 579206 -229
Branches 22645 22618 -27
==========================================
- Hits 508831 508712 -119
+ Misses 70176 70050 -126
- Partials 428 444 +16 |
| ) -> tuple[ResolvedAttribute, str | int | list[str]]: | ||
| """ | ||
| Time series request do not support virtual column contexts, so we have to remap the value back to the original column. | ||
| (see https://github.com/getsentry/eap-planning/issues/236) |
There was a problem hiding this comment.
Not sure what the usual stance is on linking to private github issues in code.
There was a problem hiding this comment.
I copied an existing comment from here: https://github.com/getsentry/sentry/pull/90343/files?diff=unified&w=0#diff-9c5eca184c0fdcb8f7f6e56f59cfc4dde2453b7d6055d431e51225d233c5e92cR512
| # Virtual context columns (VCCs) are currently only supported in TraceItemTable. | ||
| # For TopN queries, we want table and timeseries data to match. | ||
| # Here, we want to run the table request the the VCCs. SnubaParams has | ||
| # a property `is_timeseries_request` which resolves to true if granularity_secs is set. | ||
| # `is_timeseries_request` is used to evaluate if VCCs should be used. | ||
| # Unset granularity_secs, so this gets treated as a table request with | ||
| # the correct VCC. | ||
| table_query_params = params.copy() | ||
| table_query_params.granularity_secs = None |
There was a problem hiding this comment.
If this becomes a common pattern, we should look into a helper function on params that copies it and updates some fields.
There was a problem hiding this comment.
Also, does this change the behaviour of the resolver? Is that why you need to have a different resolver for the table and timeseries?
There was a problem hiding this comment.
yes - i tried to capture this in the comment:
SnubaParams has a property is_timeseries_request which resolves to true if granularity_secs is set and is_timeseries_request is used to evaluate if VCCs should be used. We want to keep this behaviour - otherwise VCCs aren't used and top events conditions end up being wrong.
tests/snuba/api/endpoints/test_organization_events_stats_span_indexed.py
Outdated
Show resolved
Hide resolved
shruthilayaj
deleted the
shruthi/fix/map-virtual-columns-properly-for-top-n-queries
branch
Chart is empty for device.class top n queries. This is because virtual context columns are currently not supported on timeseries. So we need to map it back to original column and values and back for topn [example](https://demo.sentry.io/explore/traces/?field=id&field=span.op&field=span.description&field=span.duration&field=transaction&field=timestamp&field=tags%5Bhttp.client.resolve_dns_ms%2Cnumber%5D&groupBy=device.class&mode=aggregate&project=4508968118321152&query=span.op%3Aapp.start.cold&sort=-count%28span.duration%29&statsPeriod=14d&table=span&visualize=%7B%22chartType%22%3A1%2C%22yAxes%22%3A%5B%22count%28span.duration%29%22%5D%7D)
Chart is empty for device.class top n queries.
This is because virtual context columns are currently not supported on timeseries.
So we need to map it back to original column and values and back for topn
example