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

Add support to disable attributes in ConsoleReporter to be consistent… #1092

Merged

Conversation

@husseincoder
Copy link
Contributor

@husseincoder husseincoder commented Feb 25, 2017

Add support to disable attributes in console reporter to be consistent with GraphiteReporter features .. I saw its easy to be implemented there with not so many changes

… with GraphiteReporter
Copy link
Member

@arteam arteam left a comment

Thank you for working on this! I didn't add filtering attributes to ConsoleReporter, because it's mostly used for debug purposes. But I guess there is no reason not to implement it in a backward-compatible manner. I've added a couple of comments, which need to be addressed, before merging.

* @param status Status to be logged
*/
private void printIfEnabled(MetricAttribute type, String status) {
if(isMetricAttributeDisabled(type)) {

This comment has been minimized.

@arteam

arteam Feb 27, 2017
Member

I think it would better with one condition statement. I also don't think the we need to use isMetricAttributeDisabled method as well.

if (!getDisabledMetricAttributes().contains(type)) {
    output.println(status);
}
@@ -291,8 +291,17 @@ protected boolean isShutdownExecutorOnStop() {
return shutdownExecutorOnStop;
}

protected Set<MetricAttribute> getDisabledMetricAttributes() {

This comment has been minimized.

@arteam

arteam Feb 27, 2017
Member

Please, don't remove the getDisabledMetricAttributes method. All changes must be backward-compatible.

* @param attribute metric attribute
* @return True if attribute is configured to be disabled
*/
protected boolean isMetricAttributeDisabled(MetricAttribute attribute) {

This comment has been minimized.

@arteam

arteam Feb 27, 2017
Member

I don't believe we need to extract the logic of checking of an attribute to a separate method. disabledMetricAttributes can't be null, so we basically have one method which does nothing but delegates a call to the contains method of EnumSet.

@@ -6,10 +6,7 @@
import java.io.ByteArrayOutputStream;
import java.io.PrintStream;
import java.io.UnsupportedEncodingException;
import java.util.Locale;

This comment has been minimized.

@arteam

arteam Feb 27, 2017
Member

Please, don't introduce star imports if they were not present.

.outputTo(output)
.formattedFor(Locale.US)
.withClock(clock)
.formattedFor(TimeZone.getTimeZone("PST"))

This comment has been minimized.

@arteam

arteam Feb 27, 2017
Member

I guess we could extract the timezone to a variable, so all tests use the same timezone.


final ConsoleReporter customReporter = ConsoleReporter.forRegistry(registry)
.outputTo(output)
.formattedFor(Locale.US)

This comment has been minimized.

@arteam

arteam Feb 27, 2017
Member

We could extract the locale to a variable, so all tests use the same locale.

@@ -360,7 +360,7 @@ private void reportGauge(String name, Gauge gauge) {

private void announceIfEnabled(MetricAttribute metricAttribute, String metricName, String group, double value, String units)
throws GangliaException {
if (getDisabledMetricAttributes().contains(metricAttribute)) {
if (isMetricAttributeDisabled(metricAttribute)) {

This comment has been minimized.

@arteam

arteam Feb 27, 2017
Member

Please, don't introduce unrelated changes. Let's focus on ConsoleReporter.

@@ -322,14 +322,14 @@ private void reportHistogram(String name, Histogram histogram, long timestamp) t
}

private void sendIfEnabled(MetricAttribute type, String name, double value, long timestamp) throws IOException {
if (getDisabledMetricAttributes().contains(type)){
if (isMetricAttributeDisabled(type)){

This comment has been minimized.

@arteam

arteam Feb 27, 2017
Member

Let's avoid changes in the classes not directly targeted by this pull request.

@husseincoder
Copy link
Contributor Author

@husseincoder husseincoder commented Feb 27, 2017

@arteam Thanks for the code review .. I've changed these stuff and gonna push them soon

@jplock jplock added this to the 3.2.1 milestone Feb 27, 2017
@jplock jplock added the improvement label Feb 27, 2017
@arteam arteam merged commit d170988 into dropwizard:3.2-development Feb 28, 2017
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@arteam
Copy link
Member

@arteam arteam commented Feb 28, 2017

Thanks for your hard work on this, looks great!

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

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.