Enable NewRelicMeterRegistry to publish via APM in addition to API (resolves #1761) #1854
Conversation
| @@ -52,7 +53,8 @@ def VERSIONS = [ | |||
| 'org.hdrhistogram:HdrHistogram:latest.release', | |||
| 'org.hibernate:hibernate-entitymanager:latest.release', | |||
| 'org.hsqldb:hsqldb:latest.release', | |||
| 'org.jooq:jooq:latest.release', | |||
| // Pin version temporarily to restore builds. See gh-1852 | |||
There was a problem hiding this comment.
Included so the build will pass, can be removed once #1852 is resolved
| @@ -2,6 +2,11 @@ dependencies { | |||
| api project(':micrometer-core') | |||
|
|
|||
| implementation 'org.slf4j:slf4j-api' | |||
| implementation 'com.newrelic.agent.java:newrelic-api' | |||
There was a problem hiding this comment.
Adding APM support requires the addition of a compile time dependency on the newrelic-api.
The API is fairly light, containing only annotations, interfaces, POJOs, and enums, so I don't expect this to cause any issues for consumers not using the APM integration
| * @return The NewRelic integration method | ||
| */ | ||
| default NewRelicIntegration integration() { | ||
| return NewRelicIntegration.fromString(get(prefix() + ".integration")).orElse(NewRelicIntegration.API); |
There was a problem hiding this comment.
Default to API integration to preserve existing behavior
| * POJO representation of an Insights event. | ||
| */ | ||
| // VisibleForTesting | ||
| static class NewRelicEvent { |
There was a problem hiding this comment.
To allow the APM/API integrations to share the majority of the code, a DTO to represent the event was created
| @@ -45,16 +47,16 @@ public NewRelicNamingConvention(NamingConvention delegate) { | |||
|
|
|||
| @Override | |||
| public String name(String name, Meter.Type type, @Nullable String baseUnit) { | |||
| return StringEscapeUtils.escapeJson(toValidNewRelicString(delegate.name(name, type, baseUnit))); | |||
| return toValidNewRelicString(delegate.name(name, type, baseUnit)); | |||
There was a problem hiding this comment.
With the addition of the APM integration, there was a potential issue where attributes would either be double JSON escaped, or never JSON escaped.
Removing the JSON escaping from the NamingConvention and only performing it when the data is actually converted into JSON prevents this from happening.
| */ | ||
| @SuppressWarnings("ConstantConditions") | ||
| class NewRelicMeterRegistryTest { |
There was a problem hiding this comment.
There were previously several code paths which weren't tested by this class, including conversion of various types of Meters into JSON, as well as ensuring global attributes were included.
I've ensured there's now a test for each subclass of Meter, as well as tests to ensure the global tags are applied.
| @Test | ||
| void eventFor_Gauge() { | ||
| eventFor_Gauge(this.meterNameEventTypeEnabledRegistry, ImmutableMap.<String, Object>builder() | ||
| .put("eventType", "myGauge") |
There was a problem hiding this comment.
The previous test did a simple String comparison between the generated JSON and the expected value. This broke with the addition of NewRelicEvent, because the attributes were now stored in a Map, meaning the keys were written in an non-deterministic order in the JSON.
I've refactored the tests to use Jackson to read the JSON object into a Map, and then use AssertJ's map comparator instead. That way, the tests only care that the JSON has the correct keys, values, and types, making the tests easier to write + less prone to false failures.
#1761
I've done extensive testing of this with some of my own applications, and everything appears to be working correctly.
The scope of the changes are fairly large, but I've been very careful to ensure that all of the changes should be backwards compatible for existing clients.
Specifically, all existing configurations will continue working as before, and none of the generated JSON payloads to the Insights API should change.