fix(qe): Prevent descriptions from mobc leaking into the presentation of metrics #4239
Conversation
CodSpeed Performance ReportMerging #4239 will not alter performanceComparing Summary
|
| @@ -55,22 +55,54 @@ mod smoke_tests { | |||
| .text() | |||
| .await | |||
| .unwrap(); | |||
|
|
|||
| // I would have loved to use insta in here and check the snapshot but the order of the metrics is not guaranteed | |||
There was a problem hiding this comment.
OT: We might want to guarantee the order of metrics in the future maybe then?
There was a problem hiding this comment.
I thought about it, but given that's not a requirement that is set on the exposed format. I would say that sorting wouldn't add any value for consumers of metrics, and is an additional operation to make at runtime, that would (a priori) only benefit testability.
But also given that the values of the metrics will vary from run to run, (in particular histograms recording times) we would need further processing on the response to match it with a snapshot. To the least, we would need setting numeric values to a placeholder before comparing.
We could have another approach to testing, like unmarshaling into a struct and making assertions on it, but in my opinion, doing so won't add better coverage than what we have now.
| let accepted_metric_count = query_engine_metrics::ACCEPT_LIST.len(); | ||
| let displayed_metric_count = metrics.matches("# TYPE").count(); | ||
| let non_prisma_metric_count = displayed_metric_count - metrics.matches("# TYPE prisma").count(); | ||
|
|
There was a problem hiding this comment.
OT: Maybe another assertion that we have 4 counters, 4 gauges and 3 histograms? Or is that repetitive?
| /// Describe all first-party metrics that we record in prisma-engines. Metrics recorded by third-parties | ||
| /// --like mobc-- are described by such third parties, but ignored, and replaced by the descriptions in the | ||
| /// METRICS_RENAMES map. | ||
| fn initialize_metrics_descriptions() { |
There was a problem hiding this comment.
Nit: Maybe make more explicit that those are the "first party" metrics descriptions?
| describe_histogram!( | ||
| PRISMA_DATASOURCE_QUERIES_DURATION_HISTOGRAM_MS, | ||
| "The distribution of the time datasource queries took to run" |
There was a problem hiding this comment.
Semi OT: Why are these (this one and also PRISMA_DATASOURCE_QUERIES_TOTAL done by us, and not mobc? Wouldn't it make sense for mobc to take care of everything datasource as well?
Co-authored-by: Jan Piotrowski <[email protected]>
|
Note: the test in prisma/prisma (obviously) needed to be updated, see prisma/prisma#21096 |
This is the first of set of a series of patches to improve metrics correctness and tech-debt.
I recommend seeing the modified files entirely. The code has been clarified with comments.
In this patch, the fix is to rename the metrics descriptions properly instead of leaking the descriptions coming from mobc.
I took the descriptions from the docs and changed them for clarity. I'd love a review from @ruheni in here. You can find the descriptions of each metric in the updated smoke tests, and if you agree, maybe you can tweak the docs to reflect the new descriptions.