Skip to content

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.

# 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