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

Control allocation and copy of labelNamesValues type #9241

Merged
merged 1 commit into from
Aug 11, 2022

Conversation

rtribotte
Copy link
Member

What does this PR do?

This PR makes the labelNamesValues type method `With' not mutate the original slice, by doing an explicit copy.

Motivation

Avoid potential metrics overlapping.

More

  • Added/updated tests
  • Added/updated documentation

Additional Notes

This change needs to be benchmarked.

Co-authored-by: Kevin Pollet pollet.kevin@gmail.com

@rtribotte rtribotte force-pushed the fix-prometheus-labels-names-value branch from df57f24 to 0914b8f Compare August 10, 2022 15:56
@chuckyz
Copy link
Contributor

chuckyz commented Aug 11, 2022

Just an off-hand comment but in my local testing where I confirmed the initial Prometheus metrics performance was fixed, this branch is actually showing an increase in perf vs. 2.8.1. I suspect there is some performance hit; but that it's largely offset by Golang 1.19. :)

Copy link
Member

@kevinpollet kevinpollet left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@ldez ldez left a comment

Choose a reason for hiding this comment

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

LGTM

@traefiker traefiker merged commit 4755bb2 into traefik:v2.8 Aug 11, 2022
v2 automation moved this from To review to Done Aug 11, 2022
@rtribotte rtribotte deleted the fix-prometheus-labels-names-value branch August 11, 2022 13:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
v2
Done
Development

Successfully merging this pull request may close these issues.

None yet

6 participants