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

Use pre-existing metric for log messages #9

Closed
wants to merge 1 commit into from

Conversation

bboreham
Copy link
Contributor

Someone copy-pasted this code with the result that it sometimes worked and sometimes didn't.

I think this way is better.

@coveralls
Copy link

coveralls commented Mar 13, 2018

Coverage Status

Coverage decreased (-4.3%) to 85.714% when pulling 891381c on reuse-log-metric into 0599d76 on master.

@bboreham
Copy link
Contributor Author

The test that is failing requires the old behaviour.

IDK; I think double-counting if you manage to register the same hook twice is better than unregistering something that someone else might have been using.

@marccarre
Copy link
Contributor

marccarre commented Oct 19, 2018

@bboreham, what do you now think about keeping the logic to unregister, but having namespacing, like you suggested initially in cortexproject/cortex/issues/666? That way, no double counting, and less chance of accidentally unregistering something else? Maybe promrus_log_messages_total?

@bboreham
Copy link
Contributor Author

@marccarre in the case where I hit this, the other person was deliberately trying to match the name used here, to avoid disruption. But they didn't anticipate both libraries being in the same program.

@bboreham
Copy link
Contributor Author

Very old, closing.

@bboreham bboreham closed this Nov 23, 2023
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.

3 participants