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

Rename "failures" to "failed" in qless-stats #275

Merged
merged 1 commit into from
Jan 16, 2018

Conversation

dlecocq
Copy link
Contributor

@dlecocq dlecocq commented Jan 16, 2018

Graphite/whisper cannot have a name that serves both as a metric name and as a dot-delimited prefix to another metric. Because individual failure types are reported as "failures.", the "failures" metric fails to be reported if ever there has been a report of "failures.". While this tool is not specific to graphite, it is a common tool in the monitoring ecosystem and it takes little effort to make this tool compatible.

@b4hand @evanbattaglia @kq2

Copy link
Contributor

@b4hand b4hand left a comment

Choose a reason for hiding this comment

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

I have no qualms with this change. Although, you may want to add a blurb to the README or maybe a Release Notes section somewhere that the metric name is changing. It might even be worthy of a minor version bump to 0.12.0.

@dlecocq
Copy link
Contributor Author

dlecocq commented Jan 16, 2018

I think it'd be a good idea to do at least one of those.

On the version, I have two minds about it -- it doesn't really change the core functionality, but I'd hate for peoples' alerts to break all of a sudden. I guess now that I say it aloud, probably deserves a 0.12.0 bump.

I'll add a section on release notes.

@dlecocq
Copy link
Contributor Author

dlecocq commented Jan 16, 2018

@b4hand -- updated

Graphite/whisper cannot have a name that serves both
as a metric name and as a dot-delimited prefix to
another metric. Because individual failure types are
reported as "failures.<type>", the "failures" metric
fails to be reported if ever there has been a report
of "failures.<type>". While this tool is not specific
to graphite, it is a common tool in the monitoring
ecosystem and it takes little effort to make this tool
compatible.
@dlecocq
Copy link
Contributor Author

dlecocq commented Jan 16, 2018

Build finally passed (there was one flaky build, and then Travis was being slow). Should be good to go to hit the merge button, @b4hand

@b4hand b4hand merged commit 0616a5d into seomoz:master Jan 16, 2018
@dlecocq dlecocq deleted the qless-stats-graphite branch January 17, 2018 20:08
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