-
Notifications
You must be signed in to change notification settings - Fork 1k
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 for categorical "MicrometerSample" eventType to publish New Relic metrics. #1588
Add support for categorical "MicrometerSample" eventType to publish New Relic metrics. #1588
Conversation
Previous behavior was to publish an eventType per Meter/metric name (still supported via meterNameEventTypeEnabled=true). NewRelic's own agents publish metrics to eventTypes aligned with broader categories. To align with their recommendation the default behavior is to publish metrics under a "MicrometerSample" category. When doing so, additional context is provided by including "metricName" and "metricType" attributes. Also "timeUnit" is now provided as an attribute for all Timer related metrics regardless, so Insights users can better understand time-based metrics. See: https://docs.newrelic.com/docs/insights/insights-data-sources/default-data/insights-default-data-other-new-relic-products
It seems like you are still iterating on this PR. Please comment on it when you are ready for a review. |
@checketts Hi. It's now ready for review. I had recently upgraded my IDE and forgot to adjust the tab/space indentation settings. Resulted in the extra iterations. |
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.
This looks like a good change to me, with just a few small/opinionated suggestions. I think this is an interesting step in making the existing Insights/event based exporter more useful.
It should also be noted that New Relic recently published an open source registry that supports dimensional metrics instead of events: https://github.com/newrelic/micrometer-registry-newrelic
...meter-registry-new-relic/src/test/java/io/micrometer/newrelic/NewRelicMeterRegistryTest.java
Outdated
Show resolved
Hide resolved
...meter-registry-new-relic/src/test/java/io/micrometer/newrelic/NewRelicMeterRegistryTest.java
Show resolved
Hide resolved
...icrometer-registry-new-relic/src/main/java/io/micrometer/newrelic/NewRelicMeterRegistry.java
Show resolved
Hide resolved
...icrometer-registry-new-relic/src/main/java/io/micrometer/newrelic/NewRelicMeterRegistry.java
Show resolved
Hide resolved
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.
Thanks for the tweaks!
@breedx-nr That's great. I didn't know about that. Would you be willing to update our docs to mention this new option? Docs source is at https://github.com/micrometer-metrics/micrometer-docs/ |
...icrometer-registry-new-relic/src/main/java/io/micrometer/newrelic/NewRelicMeterRegistry.java
Show resolved
Hide resolved
...icrometer-registry-new-relic/src/main/java/io/micrometer/newrelic/NewRelicMeterRegistry.java
Outdated
Show resolved
Hide resolved
Ugh. io.micrometer.spring.autoconfigure.export.prometheus.PrometheusMetricsExportAutoConfigurationTest failures. @shakuzen my simple changes are failing to build due to unrelated errors. Thoughts? |
@shakuzen I created micrometer-metrics/micrometer-docs#102 to document it based on its README file. |
Updates for New Relic properties added in Micrometer v1.3.0 related to micrometer-metrics#1588<micrometer-metrics#1588>
Updates for New Relic properties added in Micrometer v1.3.0 related to #1588
Previous behavior was to publish an eventType per Meter/metric name
(still supported via meterNameEventTypeEnabled=true). NewRelic's own
agents publish metrics to eventTypes aligned with broader categories. To
align with their recommendation the default behavior is to publish
metrics under a "MicrometerSample" category. When doing so, additional
context is provided by including "metricName" and "metricType"
attributes. Also "timeUnit" is now provided as an attribute for all
Timer related metrics regardless, so Insights users can better
understand time-based metrics.
See: https://docs.newrelic.com/docs/insights/insights-data-sources/default-data/insights-default-data-other-new-relic-products
@shakuzen
@breedx-nr