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

prom: Add option to specify metrics prefix #782

Closed
wants to merge 4 commits into from

Conversation

kellyma2
Copy link
Contributor

It's useful to be able to prefix the metrics that are being reported so as to more easily differentiate them from other applications that are using the same metrics.

This change adds the necessary bits (environment var, config file, command line) to support capturing the prefix and passing it through to the httpmetrics library.

It's useful to be able to prefix the metrics that are being reported
so as to more easily differentiate them from other applications that
are using the same metrics.

This change adds the necessary bits (environment var, config file,
command line) to support capturing the prefix and passing it through
to the httpmetrics library.
@kellyma2
Copy link
Contributor Author

@mostynb can you take a look at this?

@mostynb
Copy link
Collaborator

mostynb commented Oct 10, 2024

How disruptive would it be to just hard-code a "bazel_remote" prefix, and skip the configuration setting? And would that be sufficient, if we ignore the distruption for a moment?

@kellyma2
Copy link
Contributor Author

For folks that have dashboards and such depending on this it'd probably be fairly disruptive. If the prefix is empty it shows up as it does today which reduces disruption.

@mostynb
Copy link
Collaborator

mostynb commented Oct 12, 2024

OK, this sounds reasonable. I just have a couple of questions about the flag...

Is there a recommended value? Would "bazel_remote_" work for anyone who might want to use this setting? If so we could use a boolean flag and if there's ever a bazel-remove v3 we could switch to this behaviour and remove the flag.

@kellyma2
Copy link
Contributor Author

That is a possible option - I was mostly trying to give maximum flexibility. Any particular reason for preferring that?

@mostynb
Copy link
Collaborator

mostynb commented Oct 12, 2024

I would like to limit the number of unnecessary configuration settings, because the number of settings is growing more than I would like. If we were to remove some configuration settings in a major version upgrade in the future, then the fewer broken configurations the better, since the migration path for users would be simpler. So if there is no plausible need to specify a custom prefix there, let's not add one.

On the other hand, if you can think of a reason why we would need this configurability then that would change things a bit.

@kellyma2
Copy link
Contributor Author

Naw. That seems totally reasonable. My initial instinct was that may folks want to prefix this per instance, but after some thought I concluded that probably per application is fine.

We need to prefix by application name, not instance name.  This
provides a flag that doesn't allow for undue customization.
Copy link
Collaborator

@mostynb mostynb left a comment

Choose a reason for hiding this comment

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

This looks pretty good, I just have a couple of small questions.

utils/flags/flags.go Outdated Show resolved Hide resolved
utils/flags/flags.go Outdated Show resolved Hide resolved
@kellyma2 kellyma2 requested a review from mostynb October 17, 2024 00:36
@mostynb
Copy link
Collaborator

mostynb commented Oct 17, 2024

Thanks- I squashed this and landed it on the master branch.

@mostynb mostynb closed this Oct 17, 2024
@kellyma2 kellyma2 deleted the metrics-prefix branch October 18, 2024 06:32
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