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

Add observability check to counters #455

Merged
merged 3 commits into from
Aug 22, 2019

Conversation

AlexPadron
Copy link
Contributor

This is for issue #454

Signed-off-by: AlexPadron <alexp@kensho.com>
@AlexPadron AlexPadron force-pushed the alex-prometheus-observable branch from 63d0475 to 38cd675 Compare August 19, 2019 13:26
# metric is observable will the value be initialized.
if not self._is_observable():
raise ValueError(
'%s metric is not observable. This is likely because the metric requires '
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thinking if we want to make this more detailed, for example we could use inspect and get the method name that is being invoked. Also, we may want to use this in other metrics as well. I added the check to counters, but the same issue exists in gauges as well for example

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure "observable" is the right word to use here, it'll confuse users. Missing label values would be more accurate.

Copy link
Contributor

@brian-brazil brian-brazil left a comment

Choose a reason for hiding this comment

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

Other metric types also have helper methods that'd benefit from this.

prometheus_client/metrics.py Outdated Show resolved Hide resolved
# metric is observable will the value be initialized.
if not self._is_observable():
raise ValueError(
'%s metric is not observable. This is likely because the metric requires '
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure "observable" is the right word to use here, it'll confuse users. Missing label values would be more accurate.

@AlexPadron AlexPadron force-pushed the alex-prometheus-observable branch from 4971346 to 09c7835 Compare August 19, 2019 14:48
Signed-off-by: AlexPadron <alexp@kensho.com>
@brian-brazil
Copy link
Contributor

Other metrics types also need this, e.g. the time() functions.

Signed-off-by: AlexPadron <alexp@kensho.com>
@AlexPadron AlexPadron force-pushed the alex-prometheus-observable branch from beba64a to 4c89979 Compare August 21, 2019 13:50
@AlexPadron
Copy link
Contributor Author

Other metrics types also need this, e.g. the time() functions.

I think I got all of these (at least in metrics.py). As far as I can tell by grepping this is the only location where the metrics are defined

@brian-brazil brian-brazil merged commit dd59d9a into prometheus:master Aug 22, 2019
@brian-brazil
Copy link
Contributor

Thanks!

@AlexPadron AlexPadron deleted the alex-prometheus-observable branch August 22, 2019 17:50
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