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

Support for disabled metrics #1048

Merged
merged 4 commits into from Feb 15, 2017
Merged

Conversation

arteam
Copy link
Member

@arteam arteam commented Jan 28, 2017

This is for based on the work of @phauer in #1008. The set of disabled metrics is implemented in ScheduledReporter, so all reporters have an opportunity to implement this feature, but it's not necessarily (for instance ConsoleReporter doesn't really need it). Along with the Graphite reporter, the Ganglia reporter is also updated to support of disabling sending metrics.

@arteam arteam force-pushed the support_for_disabled_metrics branch from dc7816c to 6f2ec88 Compare Feb 4, 2017
@arteam arteam changed the title [WIP] Support for disabled metrics Support for disabled metrics Feb 4, 2017
@jplock jplock added this to the 3.2.0 milestone Feb 7, 2017
@arteam
Copy link
Member Author

arteam commented Feb 7, 2017

I think this should be good to merge. Could anybody give a review?

@ryantenney
Copy link
Member

ryantenney commented Feb 7, 2017

Looks good, except that metric types are meter, timer, counter, gauge…

@ryantenney
Copy link
Member

ryantenney commented Feb 7, 2017

And is there a reason to go with disabledMetricTypes instead of enabledMetricTypes?

@jplock jplock added the feature label Feb 7, 2017
@arteam
Copy link
Member Author

arteam commented Feb 9, 2017

And is there a reason to go with disabledMetricTypes instead of enabledMetricTypes?

I don't like the inverted logic too. But I think it makes sense from the the user's perspective. The user reports some metrics to its time-series database and sees that some of them are useless, because he/she doesn't use them. The user wants to disable them. For instance, Timer has 15 metrics, and most of them make sense; it would be strange to ask the user to pass a 14-sized set of metrics just to disable one.

@ryanrupp
Copy link

ryanrupp commented Feb 9, 2017

Is this only for the 3.2 branch or would it go into master/4.0 too? See the conversation in #837 and #178 - the idea that each "Metric" has a set of "Attributes" (name/value) off it. I'm not sure on the status of those but maybe instead of a MetricType enum, allow filtering by String based "attribute names" and just provide a helper class with the list of standard/known attribute names that currently exist (what is present in the MetricType enum currently). For 3.2 the enum makes sense as there's a fixed number of attributes, for 4.0 if #837 becomes a thing then this would just want to be changed to a String (suppose there'd be so many other breaking changes that it could just be worried about at that point). Anyway, thought of it when I saw the comment about "metric types".

@ryantenney
Copy link
Member

ryantenney commented Feb 9, 2017

@ryanrupp Good point. This would only go to 3.2, for exactly the reasons you mention. Also there will be some inversion of control with regard to reporters in v4 to make this sort of filtering easier to implement.

@arteam Ryan reminded me that we're calling these 'Attributes', can we rename MetricType and disabledMetricTypes to MetricAttribute and disabledMetricAttributes?

@arteam arteam force-pushed the support_for_disabled_metrics branch from 83a5deb to 5a72bcf Compare Feb 9, 2017
@arteam
Copy link
Member Author

arteam commented Feb 9, 2017

That's a good suggestion. Renamed MetricType to MetricAttribute.

@arteam arteam force-pushed the support_for_disabled_metrics branch 2 times, most recently from fa558b5 to d09483f Compare Feb 15, 2017
phauer and others added 4 commits Feb 15, 2017
(like all p999 across every metric). Allows fine-grained filtering of
sent metrics and therefore effective load reduction.
Add a facility to disable set of metrics which should not be reported to a
remote system. `ScheduledReporter` only saves them. The decision to
how to filter the metric is delegated to concrete reporters.
This is a more common terminology for this enumeration and it was
introduced earlier in #837.
@arteam arteam force-pushed the support_for_disabled_metrics branch from d09483f to 3ccd7a1 Compare Feb 15, 2017
@arteam
Copy link
Member Author

arteam commented Feb 15, 2017

I fixed the build (it was affected by relevant changes from the 3.1 branch moved to 3.2), now it should be good to merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants