NewRelicMeterRegistry Java Agent Insights API Support #1647

neiljpowell · 2019-10-17T03:16:27Z

This PR is for Issue #1540
NewRelicMeterRegistry support for delegating to the New Relic Java Agent Insights API for previously mentioned benefits. Construction/Builder defaults to original behavior using Insights REST API.

Implementation notes:

NewRelicClientProvider interface introduced for easy introduction of Agent support. Bulk of the original registry implementation moved to the Http client provider. Separate client provider impls will also work well for conditional Boot auto configuration based on presence of the Agent, etc. It may also help with incorporating future enhancements like dimensional metrics noted in PR Add support for categorical "MicrometerSample" eventType to publish New Relic metrics. #1588
MeterRegistryCustomizer/MeterFilter mentioned in the Issue for preventing duplicate metrics collected by the Agent would be included with the accompanying SpringBoot NewRelicMetricsExportAutoConfiguration updates.

@shakuzen @checketts @breedx-nr

Resolves #1540

- Migrate existing write/publish code to Http ClientProvider impl - Move meter output constants to interface for sharing

…eter.git into newrelic-java-agent-meter-registry

checketts · 2019-10-17T22:26:05Z

@neiljpowell I'm not too familiar with New Relic's offerings. This PR has a lot of changes for the NewRelic registry, however New Relic has created their own dimensional registry (https://github.com/newrelic/micrometer-registry-newrelic#new-relic-micrometer-registry) and has mentioned that it supersedes this one.

Can you summarize the options/approaches at a high level the New Relic users have and how this PR changes that?

neiljpowell · 2019-10-18T01:32:17Z

@checketts Hi. Yes, this enhancement provides the benefits mentioned in the originating issue noted above. It allows users of the New Relic Java agent to continue publishing metrics to Insights in a more consolidated way, that doesn't require additional API Key provisioning. Users have a high likelihood of already using the Agent and it requires its own license key. In a large enterprise, this could easily eliminate lots of extra keys. Delegating to the agent also provides buffering not provided by the current implementation. It can potentially simplify security proxy config by reducing the outbound destinations needed. @shakuzen 's preference for incorporation into the current meter registry resulted in the large amount of change. I was originally going to contribute a separate meter registry. However, the approach taken left most of the original code intact.

Regarding @breedx-nr 's announcement in my other PR, I had previously discussed this Agent based enhancement with him and other New Relic folks. They supported this Insights publishing option, as it may benefit many of their users based on points mentioned above, which may also be a consideration of users wanting to adopt the new dimensional metrics agent.

I'll get the merge conflicts resolved asap. A polish commit snuck in right after I submitted this PR.

https://github.com/micrometer-metrics/micrometer.git into newrelic-java-agent-meter-registry Conflicts: implementations/micrometer-registry-new-relic/src/main/java/io/micrometer/newrelic/NewRelicMeterRegistry.java implementations/micrometer-registry-new-relic/src/test/java/io/micrometer/newrelic/NewRelicMeterRegistryTest.java

neiljpowell · 2019-10-19T17:59:35Z

@checketts @shakuzen This PR changed the registry's baseTimeUnit to milliseconds. It was one of the few registry impls with seconds, which is an eternity when monitoring all the things. Any concerns?

checketts · 2019-10-21T15:01:23Z

which is an eternity when monitoring all the things

Would this change break user's dashboards they had already created? Does it actually impact the outputted data since you still have the decimals in seconds? Isn't 10 millis and 0.01 seconds the same?

I'm not opposed to making the change if we don't need to worry about breaking existing users. Or we should wait for a 2.x release to do the break.

neiljpowell · 2019-10-22T01:21:50Z

@checketts To ensure backward compatibility, baseTimeUnit has been reverted. It affects output.

implementations/micrometer-registry-new-relic/build.gradle

shakuzen · 2019-10-31T07:48:23Z

I've added the linked issue to the 1.4 milestone. I'll come back to more thoroughly review this in time for that. Others feel free to leave reviews in the meantime.

shakuzen

I left some review. Could you check those and rebase on the latest master when you get a chance?

...icrometer-registry-new-relic/src/main/java/io/micrometer/newrelic/NewRelicMeterRegistry.java

...crometer-registry-new-relic/src/main/java/io/micrometer/newrelic/NewRelicClientProvider.java

...-registry-new-relic/src/main/java/io/micrometer/newrelic/NewRelicHttpClientProviderImpl.java

neiljpowell · 2019-12-18T12:36:33Z

@shakuzen I've addressed all review comments. I'm happy to look at anything else you may find. Thanks.

shakuzen · 2020-03-11T06:47:06Z

@neiljpowell Sorry for the delay on this. Would you be able to rebase your changes here on the latest master so it is easier for me to merge?

neiljpowell · 2020-03-11T13:03:11Z

@shakuzen Sure. I should be able to get it done later today.

Conflicts: implementations/micrometer-registry-new-relic/build.gradle

neiljpowell · 2020-03-12T03:36:55Z

@shakuzen Master has been merged in. I had to adjust build.gradle. Please take a look.

shakuzen

Looks good to me. Thank you for contributing this and your patience on it finally landing in a release. I will polish a few things post-merge.

shakuzen · 2020-03-12T05:58:50Z

implementations/micrometer-registry-new-relic/build.gradle

@@ -2,6 +2,7 @@ dependencies {
    api project(':micrometer-core')

    implementation 'org.slf4j:slf4j-api'
+    implementation 'com.newrelic.agent.java:newrelic-api:5.+'


We want optionalApi here so we are not adding this dependency for all users of micrometer-registry-new-relic. Users who want to use the Agent client provider should include this dependency for their application.

shakuzen · 2020-03-12T06:08:04Z

...crometer-registry-new-relic/src/main/java/io/micrometer/newrelic/NewRelicClientProvider.java

+    //gauge
+    String VALUE = "value";
+    //counter
+    String THROUGHPUT = "throughput";  //TODO Why not "count"? ..confusing if just counting something


This is probably an oversight. As the philosophy of Micrometer, we want users to focus on rates of counts rather than the raw count. Accordingly, the intention was probably to ship the rate (throughput) of the counter rather than the step count. I'll open a separate issue to look at that. In this pull request, it is probably best to keep the existing behavior and limit its changes to the purpose of the pull request.

neiljpowell added 12 commits October 3, 2019 20:00

add New Relic Java Agent API dependency v5.+

6d584e3

Introduce ClientProvider interface

314433e

- Migrate existing write/publish code to Http ClientProvider impl - Move meter output constants to interface for sharing

Add Java Agent ClientProvider impl and update tests

a479eb0

re-apply builder() and start()

e0029a7

Comments

b26f516

Default meter registry Builder to Http/Rest client

53adfab

use registry.getBaseTimeUnit()

1107dc0

Move publish() up for consistency

fe79c56

Merge branch 'master' of https://github.com/micrometer-metrics/microm…

e88586d

…eter.git into newrelic-java-agent-meter-registry

clean up

2dbf891

fix formatting build errors

52ad284

fix test formatting build errors

72d5f9a

neiljpowell added 2 commits October 18, 2019 21:35

merged in polish

3e4dc70

neiljpowell added 2 commits October 21, 2019 20:04

Revert back to BaseTimeUnit Seconds

e4163ca

Merge branch 'master' into newrelic-java-agent-meter-registry

926e8cb

shakuzen reviewed Oct 31, 2019

View reviewed changes

implementations/micrometer-registry-new-relic/build.gradle Outdated Show resolved Hide resolved

neiljpowell added 2 commits October 31, 2019 17:10

Merge branch 'master' into newrelic-java-agent-meter-registry

833924e

make newrelic-api an optional dependency

e246fe1

shakuzen reviewed Dec 16, 2019

View reviewed changes

neiljpowell added 4 commits December 17, 2019 21:51

gitignore

fd8ced1

Merge branch 'master' into newrelic-java-agent-meter-registry

0d812ff

MeterRegistry updates from review

6412faf

remove redundant modifiers

e9a844e

neiljpowell added 4 commits December 17, 2019 22:40

specific arg type in sendEvents

3f89a09

reverted publish to using stream().flatMap for processing batches

6a5712f

improve http client provider publish test to ensure batching

aee20cd

improve http and agent publishing tests

5fc6f66

neiljpowell added 5 commits March 11, 2020 21:18

Merge branch 'master' into newrelic-java-agent-meter-registry

c5d0dc4

Conflicts: implementations/micrometer-registry-new-relic/build.gradle

removed optional

77ff2a9

switch newrelic-api to compileOnly

6292403

use optionalApi for newrelic-api

b1437d0

implementation newrelic-api

12ea291

shakuzen approved these changes Mar 12, 2020

View reviewed changes

shakuzen merged commit 5d5617d into micrometer-metrics:master Mar 12, 2020

This was referenced Mar 12, 2020

Enhanced support for NewRelic APM #1761

Closed

Interested in hosting legacy new-relic Meter registry? newrelic/micrometer-registry-newrelic#46

Closed

izeye mentioned this pull request Mar 17, 2020

Polish New Relic changes #1908

Merged

neiljpowell mentioned this pull request Apr 7, 2020

Migrate NewRelic metrics to NewRelicClientProvider spring-projects/spring-boot#20871

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

NewRelicMeterRegistry Java Agent Insights API Support #1647

NewRelicMeterRegistry Java Agent Insights API Support #1647

neiljpowell commented Oct 17, 2019 •

edited by shakuzen

Loading

checketts commented Oct 17, 2019 •

edited

Loading

neiljpowell commented Oct 18, 2019 •

edited

Loading

neiljpowell commented Oct 19, 2019

checketts commented Oct 21, 2019

neiljpowell commented Oct 22, 2019

shakuzen commented Oct 31, 2019

shakuzen left a comment

neiljpowell commented Dec 18, 2019

shakuzen commented Mar 11, 2020

neiljpowell commented Mar 11, 2020

neiljpowell commented Mar 12, 2020

shakuzen left a comment

shakuzen Mar 12, 2020

shakuzen Mar 12, 2020

NewRelicMeterRegistry Java Agent Insights API Support #1647

NewRelicMeterRegistry Java Agent Insights API Support #1647

Conversation

neiljpowell commented Oct 17, 2019 • edited by shakuzen Loading

checketts commented Oct 17, 2019 • edited Loading

neiljpowell commented Oct 18, 2019 • edited Loading

neiljpowell commented Oct 19, 2019

checketts commented Oct 21, 2019

neiljpowell commented Oct 22, 2019

shakuzen commented Oct 31, 2019

shakuzen left a comment

Choose a reason for hiding this comment

neiljpowell commented Dec 18, 2019

shakuzen commented Mar 11, 2020

neiljpowell commented Mar 11, 2020

neiljpowell commented Mar 12, 2020

shakuzen left a comment

Choose a reason for hiding this comment

shakuzen Mar 12, 2020

Choose a reason for hiding this comment

shakuzen Mar 12, 2020

Choose a reason for hiding this comment

neiljpowell commented Oct 17, 2019 •

edited by shakuzen

Loading

checketts commented Oct 17, 2019 •

edited

Loading

neiljpowell commented Oct 18, 2019 •

edited

Loading