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

Automatically initialize time series without labels #209

Merged
merged 1 commit into from
Mar 24, 2021

Conversation

dmagliola
Copy link
Collaborator

@dmagliola dmagliola commented Dec 24, 2020

Issue #208

According to Prometheus Best Practices, client libraries are expected to automatically export a 0 value when declaring a metric that has no labels.

We missed this so far in the Ruby client, this PR rectifies that.

NOTE: This can be considered a breaking change. On the one hand, it's a bug fix, but on the other, it will make many time series materialize when scraping apps that use the client, that previously wouldn't have.

Depending on how many label-less metrics the app is declaring, this may have a significant impact, so we should probably cut a new major version and warn about this in the Release Notes.

@dmagliola dmagliola changed the title Automatically initialize time series without labels (Issue #208) Automatically initialize time series without labels Dec 24, 2020
According to [Prometheus Best Practices](https://prometheus.io/docs/practices/instrumentation/#avoid-missing-metrics),
client libraries are expected to automatically export a 0 value when declaring
a metric that has no labels.

We missed this so far in the Ruby client, this PR rectifies that.

NOTE: This can be considered a breaking change. On the one hand, it's a bug fix,
but on the other, it will make many time series materialize when scraping apps
that use the client, that previously wouldn't have.

Depending on how many label-less metrics the app is declaring, this may have a
significant impact, so we should probably cut a new major version and warn
about this in the Release Notes.

Signed-off-by: Daniel Magliola <dmagliola@crystalgears.com>
@dmagliola dmagliola force-pushed the init_metrics_without_labels branch from dc5c059 to feccff3 Compare December 24, 2020 18:40
@dmagliola dmagliola requested a review from Sinjo December 24, 2020 18:41
@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling feccff3 on init_metrics_without_labels into 4cdd0ab on master.

@coveralls
Copy link

coveralls commented Dec 24, 2020

Coverage Status

Coverage remained the same at 100.0% when pulling feccff3 on init_metrics_without_labels into 4cdd0ab on master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling feccff3 on init_metrics_without_labels into 4cdd0ab on master.

Copy link
Member

@Sinjo Sinjo left a comment

Choose a reason for hiding this comment

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

I'm 👍🏻 on this, and I think we should go with a major bump as a courtesy. Hitting capacity on your Prometheus database is not a fun experience (often friends have reported just dropping all their metric history when this happens, which is kinda sad). If we can avoid ruining people's day like that for some thing as cheap as bumping the major version number, we should.

@Sinjo Sinjo added this to the v3.0.0 milestone Mar 24, 2021
@Sinjo
Copy link
Member

Sinjo commented Mar 24, 2021

I've added this to a v3.0.0 milestone. If we have any other changes (either outstanding PRs or issues that won't take us much work - so probably not OpenMetrics) we think need the major version bump we should bundle them in there.

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