Skip to content

Rename failure metrics #65

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

Merged
merged 4 commits into from
Oct 24, 2016
Merged

Rename failure metrics #65

merged 4 commits into from
Oct 24, 2016

Conversation

jml
Copy link
Contributor

@jml jml commented Oct 24, 2016

Fixes #58


This change is Reviewable

@juliusv
Copy link
Contributor

juliusv commented Oct 24, 2016

Oh wow. What. I was wondering how this was even possible, given that the metrics registry should complain about two metrics with the same name. Turns out, I just didn't even collect those metrics at all :( Shame on me. Could you also add the missing four ingester metrics to the Describe() and Collect() methods of the distributor?

@jml
Copy link
Contributor Author

jml commented Oct 24, 2016

Yeah, was going to do that in a different PR but can do it here also.

@jml jml force-pushed the 58-metric-names branch from 9a94386 to 6f01040 Compare October 24, 2016 14:03
"The percent ownership of the ring by ingester",
[]string{"ingester"}, nil,
),
ingesterTotalDesc: prometheus.NewDesc(
"prometheus_distributor_ingesters_total",
"prism_distributor_ingesters_total",
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, while you're at it: this shouldn't have a _total suffix, as it's a gauge, not a counter. Just prism_distributor_ingesters.

"Number of ingesters in the ring",
nil, nil,
),
tokensTotalDesc: prometheus.NewDesc(
"prometheus_distributor_tokens_total",
"prism_distributor_tokens_total",
Copy link
Contributor

Choose a reason for hiding this comment

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

Same.

@juliusv
Copy link
Contributor

juliusv commented Oct 24, 2016

👍 otherwise.

@jml jml mentioned this pull request Oct 24, 2016
@jml jml merged commit a8fbd70 into master Oct 24, 2016
@jml jml deleted the 58-metric-names branch October 24, 2016 17:59
tomwilkie added a commit that referenced this pull request Jan 21, 2017
b783528 Tweak test script so it can be run on a mca
a3b18bf Merge pull request #65 from weaveworks/fix-integration-tests
ecb5602 Fix integration tests
f9dcbf6 ... without tab (clearly not my day)
a6215c3 Add break I forgot
0e6832d Remove incorrectly added tab
eb26c68 Merge pull request #64 from weaveworks/remove-test-package-linting
f088e83 Review feedback
2c6e83e Remove test package linting

git-subtree-dir: tools
git-subtree-split: b783528b75f6ba52a51832bd087bd8347b26f131
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