-
Notifications
You must be signed in to change notification settings - Fork 6
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
Generate metric name methods for all metrics #1007
Conversation
Generate changelog in
|
metric-schema-java/src/main/java/com/palantir/metric/schema/UtilityGenerator.java
Show resolved
Hide resolved
@@ -564,18 +574,19 @@ private static void generateMetricFactoryBuilder( | |||
// TODO(ckozak): Update to use a method which can log a warning and replace existing gauges. | |||
// See MetricRegistries.registerWithReplacement. | |||
.addStatement( | |||
"$L.$L($L(), $L)", | |||
"$L.$L($N(), $L)", |
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.
I think these should all have been $N
because they're name references, shouldn't make much difference in practice outside of cases like this where we pass a *Spec. Up to you if you'd like to replace them here or not.
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.
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.
looks good to me, thanks! Feel free to add merge-when-ready at your convenience
👍 |
Released 0.21.0 |
Continuation of #293, but exposing this capability for all metrics.
I think the risk of misuse is minimal. Consumers can always bypass metric-schema entirely if they want. But if they want/need to off-road, it's better to at least have the metrics documented in metric-schema.
There are a couple places in our internal server library where this would be useful.