-
Notifications
You must be signed in to change notification settings - Fork 98
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
Aggregate dimensional metrics #408
Aggregate dimensional metrics #408
Conversation
👀 |
I have signed the CLA 👎🏼 I must have done it from my NR laptop 🙃 |
I was able to send But I'm having issues with |
lib/new_relic/harvest/telemetry_sdk/dimensional_metrics/harvester.ex
Outdated
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.
Great stuff, thanks for picking this up!
confirmed |
lib/new_relic/harvest/telemetry_sdk/dimensional_metrics/harvester.ex
Outdated
Show resolved
Hide resolved
lib/new_relic/harvest/telemetry_sdk/dimensional_metrics/harvester.ex
Outdated
Show resolved
Hide resolved
lib/new_relic/harvest/telemetry_sdk/dimensional_metrics/harvester.ex
Outdated
Show resolved
Hide resolved
lib/new_relic/harvest/telemetry_sdk/dimensional_metrics/harvester.ex
Outdated
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.
Looks great. Sort of back tracked on how to handle the nil attribute thing (see comment) but otherwise this looks good to me! I see Tony already gave it the thumbs up.
We should give it a go in some prod systems to see how it goes.
Did you test the memory stuff with large maps in the key? I think that was the only remaining open question
This is a stab to build off of this PR #376
Looking at the original comments by Vince, I think one missing piece was aggregating the dimensional metrics during the 5 second reporting interval? I took a stab at it.@tpitale , if you have some time, love to hear your thoughts on this. I know @mattbaker is interested in getting it over the finish line.
Also, I think that common block is accurate for
count
andsummary
- https://docs.newrelic.com/docs/data-apis/ingest-apis/metric-api/report-metrics-metric-api/#optional-map-attributes.But for
gauge
, it appears the interval.ms value isn't required. It probably does no harm 😄 As an aside, the old NR Telemetry SDK specs also showgauge
without interval.ms