Workaround for __name__ grouping in avg() #280
Conversation
| @@ -115,3 +115,9 @@ func CloneExpr(expr promql.Expr) (newExpr promql.Expr) { | |||
| newExpr, _ = promql.ParseExpr(expr.String()) | |||
| return | |||
| } | |||
|
|
|||
| // PreserveLabel wraps the input expression with a label replace in order to preserve the metadata through binary expressions | |||
| func PreserveLabel(expr promql.Expr, srcLabel string, dstLabel string) (relabelExpress promql.Expr) { | |||
There was a problem hiding this comment.
I'd like to find a way to create this object without having to string and unstring everything if possible (this works, just feels nasty)
There was a problem hiding this comment.
I've looked a bit into this and the promql package doesn't make this easy. I also ran a benchmark and it seems that the perf hit is non-existant:
benchmark old ns/op new ns/op delta
BenchmarkEvaluations/benchdata/aggregators.test/direct-4 20803 20102 -3.37%
BenchmarkEvaluations/benchdata/aggregators.test/promxy-4 232304 226409 -2.54%
BenchmarkEvaluations/benchdata/aggregators.test/promxy_remoteread-4 233642 231085 -1.09%
benchmark old allocs new allocs delta
BenchmarkEvaluations/benchdata/aggregators.test/direct-4 143 143 +0.00%
BenchmarkEvaluations/benchdata/aggregators.test/promxy-4 572 571 -0.17%
BenchmarkEvaluations/benchdata/aggregators.test/promxy_remoteread-4 569 569 +0.00%
benchmark old bytes new bytes delta
BenchmarkEvaluations/benchdata/aggregators.test/direct-4 7517 7517 +0.00%
BenchmarkEvaluations/benchdata/aggregators.test/promxy-4 37130 36696 -1.17%
BenchmarkEvaluations/benchdata/aggregators.test/promxy_remoteread-4 36737 36776 +0.11%
So assuming tests pass, I'll rebase/squash and we should be good to go.
There was a problem hiding this comment.
I'm new to these libraries so I'll continue to dig for a better option. I'm not sure if there is a better way to run the label_replace function as it has to be outside of the aggregate and not inside.
There was a problem hiding this comment.
Besides the fact that this is ugly (String-ing and re-parsing) this seems fine; I was worried that the re-parse would be dramatically slower, but it seems its not noticeable in the scheme of things :)
Right now promql's BinaryExpr strips the __name__ label -- which breaks the avg() optimizations in the NodeReplacer. This patch implements a relabel workaround (which is nasty, but works) to relabel __name__ -> __name and then back. Fixes #274 Special thanks to @john-delivuk for the debugging help!
Right now promql's BinaryExpr strips the name label -- which breaks
the avg() optimizations in the NodeReplacer. This patch implements a
relabel workaround (which is nasty, but works) to relabel
__name__->__nameand then back.Fixes #274
Special thanks to @john-delivuk for the debugging help!
TODO: