-
Notifications
You must be signed in to change notification settings - Fork 4.1k
[STORM-1590] port defmeters/defgauge/defhistogram... to java for all of our code to use #1171
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
Conversation
| @SuppressWarnings("unchecked") | ||
| public class StormMetricsRegistry { | ||
| private static final Logger LOG = LoggerFactory.getLogger(StormMetricsRegistry.class); | ||
| private static final MetricRegistry metrics = new MetricRegistry(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we just make this public and rename it to DEFAULT_REGISTRY. We can still use the helper methods, but they are not as critical.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, will change
|
One minor comment. I am +1 either way. |
2. change register-metrics-reporter/reporters in common to java
|
@revans2 changed according to your comments, also changed all the other files. start-metrics-reporters in common.clj is changed to java too. Please help review again. |
|
@revans2 @unsleepy22 - can we use (.) as separator instead of (:) in the metric names? Reason being that graphite and possibly other systems recognize dot as a separator but not (:)? It need not be done in this PR itself. I can make those changes in a separate PR. |
|
@abhishekagarwal87 I would be fine with a change like that. Please file a JIRA for it and we can make the change everywhere. |
|
@unsleepy22 thank you +1 |
|
@revans2 could you take a look? |
|
+1 looks good to me |
This PR is under STORM-1590, I changed nimbus.clj only currently and would like to hear your opinion first before moving on.