Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MQE: Add support for round #9651

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

jhesketh
Copy link
Contributor

@jhesketh jhesketh commented Oct 17, 2024

What this PR does

Which issue(s) this PR fixes or relates to

Fixes #

Checklist

  • Tests updated.
  • Documentation added.
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX].
  • about-versioning.md updated with experimental features.

@jhesketh jhesketh requested a review from a team as a code owner October 17, 2024 04:54
@jhesketh
Copy link
Contributor Author

(note that I'll update the point indexes in a separate change because I want discuss an implementation specific)

pkg/streamingpromql/testdata/ours/functions.test Outdated Show resolved Hide resolved

f := functions.FunctionOverInstantVector{
SeriesDataFunc: functions.Round,
// TODO(jhesketh): With the currently vendored prometheus, round does not consistently drop the __name__ label
Copy link
Contributor

Choose a reason for hiding this comment

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

Does not consistently drop the __name__ label, or doesn't drop it at all?

Is there an issue link for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is tricky. Against main, round does not return any name (as evidence by the tests here prometheus/prometheus#15176).

Against what we have vendored it does have the name. So the bug has seemingly since been fixed. I have not found an exact issue but assume it is related to the more recent delayed label name dropping work fixing something incidentally.

Also worth noting is that in the vendored version it has DropName: true,, so I'm not sure if that isn't being honoured somewhere (I haven't tracked down where).

The only upstream tests of round are in the aggregations.test file, such as:

eval instant at 50m round(0.005 * http_requests{group="production",job="api-server"})
	{group="production", instance="0", job="api-server"} 1
	{group="production", instance="1", job="api-server"} 1

Which due to the multiplication has the resulting label dropped.

So it wouldn't surprise me if it went unnoticed upstream and also fixed as part of the label work (also unnoticed).

Copy link
Contributor

@charleskorn charleskorn Oct 17, 2024

Choose a reason for hiding this comment

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

Right, thanks for the explanation.

Also worth noting is that in the vendored version it has DropName: true,, so I'm not sure if that isn't being honoured somewhere (I haven't tracked down where).

DropName is only used when the experimental new delayed __name__ dropping is enabled, and this is disabled by default.

I would rephrase the comment slightly to make it clearer, something like this perhaps:

Suggested change
// TODO(jhesketh): With the currently vendored prometheus, round does not consistently drop the __name__ label
// TODO(jhesketh): With the version of Prometheus vendored at the time of writing, round does not drop the __name__ label, and this is verified by our tests.
// We match this behaviour for consistency, but this behaviour has changed in Prometheus 3.0, so we'll need to match that once Prometheus 3.0 is vendored in.

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