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.

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

[pkg/translator/prw] Use flag to control metrics normalization. #22040

Closed
wants to merge 2 commits into from
Closed

[pkg/translator/prw] Use flag to control metrics normalization. #22040

wants to merge 2 commits into from

Conversation

kovrus
Copy link
Member

@kovrus kovrus commented May 17, 2023 •
edited
Loading

Description:

Use flag to control metrics normalization. It is more convenient to control metrics name normalization via a flag
when pkg/translator/prometheusremotewrite is used as a third-party dependency.

Link to tracking Issue:
#22040

Testing:
Exisitng unit tests

@kovrus kovrus marked this pull request as ready for review May 17, 2023 13:22
@kovrus kovrus requested review from a team and dashpole as code owners May 17, 2023 13:22
@kovrus kovrus marked this pull request as draft May 17, 2023 13:27
@kovrus kovrus marked this pull request as ready for review May 17, 2023 22:03
@kovrus
Copy link
Member Author

kovrus commented May 23, 2023

@jpkrohling would you have a moment to take a look at it?

jpkrohling
jpkrohling approved these changes May 25, 2023
View reviewed changes
Copy link
Member

@jpkrohling jpkrohling left a comment

Choose a reason for hiding this comment

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

This makes way more sense than the current state!

@kovrus
Copy link
Member Author

kovrus commented May 25, 2023

@Aneurysm9 can you please take a look as a code owner?

@kovrus kovrus removed the request for review from rapphil May 25, 2023 12:55
Aneurysm9
Aneurysm9 suggested changes May 25, 2023
View reviewed changes
Copy link
Member

@Aneurysm9 Aneurysm9 left a comment

Choose a reason for hiding this comment

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

I believe this needs to be considered in conjunction with #21743.

pkg/translator/prometheus/normalize_name.go Outdated Show resolved Hide resolved
@github-actions github-actions bot requested a review from rapphil May 25, 2023 19:37
@kovrus
Copy link
Member Author

kovrus commented May 25, 2023 •
edited
Loading

I believe this needs to be considered in conjunction with #21743.

@Aneurysm9 I am not sure about that. This PR is an attempt to drop the dependency of pkg/translator/prw on the normalize name feature gate, it is independent of what the default should be or anything else. It is just a code restructuring so this module can be used as a third-party dependency elsewhere without using the feature gate to configure its behavior. I think #21743 should not be a blocker here.

@kovrus kovrus force-pushed the normalization-via-flag branch 2 times, most recently from bf90bf3 to cfd4895 Compare May 25, 2023 19:48
@kovrus kovrus requested a review from Aneurysm9 May 25, 2023 19:49
@github-actions
Copy link
Contributor

This PR was marked stale due to lack of activity. It will be closed in 14 days.

@github-actions github-actions bot added the Stale label Jun 10, 2023
@kovrus kovrus removed the Stale label Jun 11, 2023
@github-actions
Copy link
Contributor

This PR was marked stale due to lack of activity. It will be closed in 14 days.

@github-actions github-actions bot added the Stale label Jun 26, 2023
@mx-psi mx-psi removed the Stale label Jun 26, 2023
@kovrus kovrus closed this Jul 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jpkrohling jpkrohling jpkrohling approved these changes

@gouthamve gouthamve Awaiting requested review from gouthamve

@dashpole dashpole Awaiting requested review from dashpole dashpole is a code owner

@rapphil rapphil Awaiting requested review from rapphil

@Aneurysm9 Aneurysm9 Awaiting requested review from Aneurysm9

Assignees

@mx-psi mx-psi

Labels
exporter/prometheusremotewrite pkg/translator/prometheus
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@kovrus @jpkrohling @Aneurysm9 @atoulme @mx-psi