Skip to content

Conversation

fpetkovski
Copy link
Contributor

This commit adds a PromQL benchmark for queries on native histograms.

Copy link
Member

@bboreham bboreham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems plausible.

return err
}
}
if _, err := app.AppendHistogram(0, labels.FromStrings(lbls...), ts, histograms[i], nil); err != nil {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Calling FromStrings repeatedly could be expensive.
I see there are similar things in other tests and can't point at an obviously better example.
There's this, but really it ought to re-use the Builder.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense. I've extracted the call to labels.FromStrings at the top of the loop. We still call it once per series, but at least it's not once per sample.

@beorn7 beorn7 self-requested a review November 21, 2023 13:48
@fpetkovski fpetkovski force-pushed the native-histogram-benchmark branch from e1f979e to 0dbb3de Compare November 22, 2023 12:23
@bboreham
Copy link
Member

Lint issue should be fixed now if you rebase.

This commit adds a PromQL benchmark for queries on native histograms.

Signed-off-by: Filip Petkovski <filip.petkovsky@gmail.com>
Signed-off-by: Filip Petkovski <filip.petkovsky@gmail.com>
Signed-off-by: Filip Petkovski <filip.petkovsky@gmail.com>
@fpetkovski fpetkovski force-pushed the native-histogram-benchmark branch from bfb48a5 to 792b824 Compare November 23, 2023 12:02
@bboreham bboreham merged commit 35a15e8 into prometheus:main Nov 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants