Skip to content

Upgrade client_golang to 1.21.0-rc.0 (and common to 0.62.0 with model.NameValidationScheme breaking change) #15851

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 5 commits into from
Jan 27, 2025
Merged

Upgrade client_golang to 1.21.0-rc.0 (and common to 0.62.0 with model.NameValidationScheme breaking change) #15851

merged 5 commits into from
Jan 27, 2025

Conversation

bwplotka
Copy link
Member

@bwplotka bwplotka commented Jan 21, 2025 •
edited
Loading

This tests 1.21.0-rc.0 as per https://github.com/prometheus/client_golang/blob/main/RELEASE.md

We could merge it too as an early adopter.

Known effects (as I had to change tests):

  • relabel: This allows UTF-8 in relabelling Replace + TargetLabel with ${} vars.
  • rule_fmt: Rule label names can now contain UTF-8 chars. Record name also can contain UTF-8 char. This makes it ambiguous if record: metric{label="val"} is a correct metric name or mistake (should be in expr). I opted for disallowing {} chars and catch this (potential) mistake. We can relax it later if needed...

Given this replaces a global var, do you mind double checking this PR @ywwg @beorn7 ?

@bwplotka
Upgrade client_golang to 1.21.0-rc.0
de00608 
This tests 1.21.0-rc.0 as per https://github.com/prometheus/client_golang/blob/main/RELEASE.md

We could merge it too as an early adopter.

Signed-off-by: bwplotka <bwplotka@gmail.com>
@bwplotka bwplotka changed the title Upgrade client_golang to 1.21.0-rc.0 Upgrade client_golang to 1.21.0-rc.0 (and common to 0.62.0 with model.NameValidationScheme breaking change) Jan 21, 2025
kakkoyun
kakkoyun approved these changes Jan 21, 2025
View reviewed changes
Copy link
Member

@kakkoyun kakkoyun left a comment

Choose a reason for hiding this comment

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

LGTM

@ArthurSens
Copy link
Member

With that many CI failures, it looks likely that the dependency update broke something 🤔

@bwplotka
Copy link
Member Author

bwplotka commented Jan 22, 2025 •
edited
Loading

It's kind of expected for Prometheus which carefully checks metric chars etc, but yea it's going to be a painful.

@bwplotka
Fixed relabelling; allowing UTF-8 in targetLabel.
7263dfe 
Signed-off-by: bwplotka <bwplotka@gmail.com>
@bwplotka bwplotka requested review from ywwg and beorn7 January 23, 2025 08:54
@bwplotka
Fixed rulefmt UTF-8 expectations.
80d702a 
Signed-off-by: bwplotka <bwplotka@gmail.com>
beorn7
beorn7 reviewed Jan 23, 2025
View reviewed changes
Copy link
Member

@beorn7 beorn7 left a comment

Choose a reason for hiding this comment

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

Generally looks good to me, but I'm not the expert here. Needs others to approve.

Note that this is not just a dependency update, but fixes bugs that have been there before the update. (We just note them now by making UTF-8 support the default.)

@beorn7
Copy link
Member

beorn7 commented Jan 23, 2025

Is it even possible now to write a target label that contains a literal ${1} or $1 that is not support to be a capture group reference? Can you escape the $? If so, should we document that behavior? If not, should we make it possible?

@bwplotka
Copy link
Member Author

Both rulefmt and relabel decisions are not trivial, so I am happy to keep the old behaviour for this PR and create issues to tackle later.

@beorn7
Copy link
Member

beorn7 commented Jan 23, 2025

Maybe let @ywwg decide how and in which order best to tackle these.

@bwplotka
Copy link
Member Author

Yup, I pinged Owen on slack too.

@ywwg
Copy link
Member

ywwg commented Jan 24, 2025

looking now sorry for the delay

@ywwg
Copy link
Member

ywwg commented Jan 24, 2025

imho, it seems unlikely that someone will want to make $1 a metric name, even though that is allowed by the new standard. I am ok disallowing this pattern, and if in 2 years someone comes along with a client that absolutely must have that metric name they can rightly argue that it should be allowed and a new escaping can be added. I don't think we should over-complicate implementations based on extremely hypothetical corner cases.

ywwg
ywwg requested changes Jan 24, 2025
View reviewed changes
Copy link
Member

@ywwg ywwg left a comment

Choose a reason for hiding this comment

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

looks good overall, thank you

bwplotka and others added 2 commits January 24, 2025 15:49
@bwplotka @ywwg
Update model/rulefmt/rulefmt.go
bd0d9e7 
Co-authored-by: Owen Williams <owen.williams@grafana.com>
Signed-off-by: Bartlomiej Plotka <bwplotka@gmail.com>
@bwplotka
Addressed comments.
36cf85f 
Signed-off-by: bwplotka <bwplotka@gmail.com>
@bwplotka bwplotka requested a review from ywwg January 27, 2025 09:58
@bwplotka bwplotka merged commit 6ede900 into main Jan 27, 2025
44 checks passed
@bwplotka bwplotka deleted the upgradeclient branch January 27, 2025 15:52
@bwplotka bwplotka mentioned this pull request Jan 30, 2025
Merged
@bwplotka bwplotka mentioned this pull request Feb 17, 2025
Merged
@simonpasquier simonpasquier mentioned this pull request Feb 19, 2025
Closed
5 tasks
This was referenced Mar 2, 2025
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@beorn7 beorn7 beorn7 left review comments

@kakkoyun kakkoyun kakkoyun approved these changes

@ywwg ywwg ywwg approved these changes

@ArthurSens ArthurSens Awaiting requested review from ArthurSens

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@bwplotka @ArthurSens @beorn7 @ywwg @kakkoyun