Add rate aggFn support for sum metrics #77
Conversation
🦋 Changeset detectedLatest commit: 38215d1 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
| SELECT | ||
| if( | ||
| runningDifference(value) < 0 | ||
| OR neighbor(_string_attributes, -1, _string_attributes) != _string_attributes, |
There was a problem hiding this comment.
I assume this handles the edge cases. I wonder if this map comparison is strict enough or we need to sort keys first
There was a problem hiding this comment.
Ah that's a good point, do you think it's something we should do in the query or at ingestion time?
There was a problem hiding this comment.
It looks like sorting is done on the SDK side today. Prometheus looks to operate under the same assumption so I think we're okay here.
| AND data_type = ? | ||
| AND (?) | ||
| GROUP BY | ||
| _string_attributes, |
There was a problem hiding this comment.
can we also group by the name here ?
There was a problem hiding this comment.
Since name is fixed, it didn't seem necessary so I just did min(name), though now I wonder if group by name is actually more performant than min(name). I think right now the query is correct - but I realize that there's possibly another option where we just inject the name as a fixed value in the query itself so Clickhouse doesn't have to do any additional processing on the name column as well.
On the perf side - I'm realizing that the indexing order has an effect here on the grouping as well (and it seems like a load of other things), and we can probably come back and do some benchmarking/see if there's an optimization to be done here later as CH's grouping optimization logic is quite complex.
Found a few things that we'd likely need to clean up for #76, but I think this makes a huge improvement in making sum metrics usable.