feat(MEP): Implement the run_query method#31652
Conversation
- This adds a MetricsQueryBuilder, which works very similarily to our
QueryBuilder, but with specific handlers for how metrics construct
queries
- This MetricsQueryBuilder does not yet construct snql queries, and will
not because table queries will require multiple queries to construct
similar table data
- that is, if we want [transaction, p95, count_unique(user)], we need
a query against distributions with [transaction, p95] followed by a
second query for [transaction, count_unique(user)] against the sets
table
- This is so we can maintain a sortby
- This checks the orderby for each entity, and orders the queries so the
entity with the most orderbys are first.
- If more than one orderby contains functions throw an error since the
query is now impossible
- This then uses the groupby to group the results of the up to 3 queries
back together
|
Moving to draft, need to rework the conditions for 2nd+ queries |
- This also adds better tests since the previous ones were not sufficient
k-fish
left a comment
k-fish
left a comment
There was a problem hiding this comment.
This is fine, but some of the code should be split into functions for clarity either now or in the future, and it could use more tests. Should also get SnS review for how the query is being built if they have a chance.
evanh
left a comment
evanh
left a comment
There was a problem hiding this comment.
This idea makes sense. Couple questions. Also I don't know if you planned to add this now or were going to do it later, but I would think about how to instrument this, particularly the results aggregation.
| ], | ||
| ), | ||
| Op.IN, | ||
| Function("tuple", groupby_values), |
There was a problem hiding this comment.
It seems like groupby_values is unbounded. Is there a limit to how big this list can be?
There was a problem hiding this comment.
Good point, We probably don't want to allow doing the 10k limit on these table queries. I'll enforce a 51 limit on this builder for now since we only plan to use this for the performance table 👍
There was a problem hiding this comment.
Also keep in mind there is a maximum size of the actual query you can build. So if you have 100k conditions, the SQL string will be too large for Clickhouse to process.
| groupby_values.append(groupby_key) | ||
| value_map[value_map_key].update(row) | ||
| result["meta"] += current_result["meta"] | ||
| result["data"] = list(value_map.values()) |
There was a problem hiding this comment.
Something I thought of: metrics makes no guarantees that there will be data for every aggregation for every group by. This is a contrived example, but there could be a p95 for transaction x but not unique users. Is your code supposed to handle that case?
There was a problem hiding this comment.
yes 👍 I'll add an explicit test for this, but what should happen is that we'll get p95 for x, and nothing for count_unique.
There was a problem hiding this comment.
Ah, very good point, i just realized for this scenario if we sort by unique users we'll get an unexpected result (x just won't show up at all), and I don't think there's a workaround either 🤔 Going to document this as a known deficiency for now.
|
re: instrumenting, I'll tackle that when its hooked up to an endpoint. I find it easier to do at that point since I'll be able to see the spans in context, thanks for the reminder! 🙏 |
- Adding a max limit to the metric query builder since we don't ever
want to do the group by filtering on more than a reasonable number
- Picking 51 for now since that aligns with tables in the UI (+1 for
pagination)
- Adding a test & skipped test with note for the case where the data is
sparse between the tables (ie. only in distribution but not set)
| result = query.run_query("test_query") | ||
| assert len(result["data"]) == 2 | ||
| assert result["data"][0] == { | ||
| "transaction": indexer.resolve("foo_transaction"), |
There was a problem hiding this comment.
The result here is an integer, right? Should it be mapped back to the string for the response?
There was a problem hiding this comment.
TBH, still haven't decided if that makes more sense in a post-processing step, or here in the run_query function. Going to leave this for now and revisit when I connect this to an endpoint.
| ) | ||
| current_result = raw_snql_query( | ||
| query, | ||
| referrer, |
There was a problem hiding this comment.
I thought about this and I think I would do a separate referrer to delineate between the "primary" and secondary queries, since the conditions will change between those two types of queries.
4 participants
Uh oh!
There was an error while loading. Please reload this page.