Skip to content
This repository has been archived by the owner on Aug 6, 2024. It is now read-only.

Add build properties file in its own namespace to carry project versi… #78

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
+78 −0
Open

Add build properties file in its own namespace to carry project versi… #78

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
5f08e04
Add build properties file in its own namespace to carry project versi…
akodali18 Aug 25, 2020
84a2092
Fix review comments. Included specific resources to filter, move our …
akodali18 Oct 8, 2020
81d3722
Add check to unit test to verify verion gauge is exported.
akodali18 Oct 12, 2020
cd3e588
revert optimize import orders.
akodali18 Oct 12, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions wavefront-spring-boot-bom/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,21 @@
</dependencies>
</dependencyManagement>
<build>
<resources>
<resource>
<directory>src/main/resources</directory>
<excludes>
<exclude>META-INF/wavefront-spring-boot/*.properties</exclude>
</excludes>
</resource>
<resource>
<directory>src/main/resources</directory>
<filtering>true</filtering>
akodali18 marked this conversation as resolved.
Show resolved Hide resolved
<includes>
<include>META-INF/wavefront-spring-boot/*.properties</include>
</includes>
</resource>
</resources>
<plugins>
<plugin>
<groupId>org.codehaus.mojo</groupId>
Expand Down
18 changes: 18 additions & 0 deletions ...-boot/src/main/java/com/wavefront/spring/autoconfigure/WavefrontMetricsConfiguration.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,15 @@
package com.wavefront.spring.autoconfigure;

import java.util.Collections;
import java.util.HashMap;
import java.util.Map;
import java.util.concurrent.TimeUnit;
import java.util.stream.Collectors;

import com.wavefront.internal.reporter.WavefrontInternalReporter;
import com.wavefront.internal_reporter_java.io.dropwizard.metrics5.MetricName;
import com.wavefront.sdk.appagent.jvm.reporter.WavefrontJvmReporter;
import com.wavefront.sdk.common.Utils;
import com.wavefront.sdk.common.WavefrontSender;
import com.wavefront.sdk.common.application.ApplicationTags;
import io.micrometer.core.instrument.Tag;
Expand All @@ -31,6 +36,7 @@
@Configuration(proxyBeanMethods = false)
@ConditionalOnBean(WavefrontSender.class)
class WavefrontMetricsConfiguration {
public static final String SDK_INTERNAL_METRIC_PREFIX = "~sdk.java.wavefront_spring_boot_starter";

@Bean
@ConditionalOnMissingBean
Expand All @@ -43,6 +49,18 @@ WavefrontJvmReporter wavefrontJvmReporter(WavefrontSender wavefrontSender, Appli
return reporter;
}

@Bean
@ConditionalOnMissingBean
WavefrontInternalReporter wavefrontInternalReporter(WavefrontSender wavefrontSender,
akodali18 marked this conversation as resolved.
Show resolved Hide resolved
WavefrontConfig wavefrontConfig) {
WavefrontInternalReporter reporter = new WavefrontInternalReporter.Builder().
prefixedWith(SDK_INTERNAL_METRIC_PREFIX).withSource(wavefrontConfig.source()).
build(wavefrontSender);
Double sdkVersion = Utils.getSemVerGauge("wavefront-spring-boot");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How is, for example, the current version 2.0.1 represented as a Double?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2.0.1 will be represented as 2.0001.
Please refer to below java doc for more details.
https://github.com/wavefrontHQ/wavefront-sdk-java/blob/0d7263daf47d06c118632d5652d2269863467f80/src/main/java/com/wavefront/sdk/common/Utils.java#L590-L602

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I see. I find it odd, but since this is internal for wavefront, if it works for you all then that's your business. I do worry that in communications between the wavefront team and others, this could be confusing if someone mentions the version of the wavefront-spring-boot-starter as 2.0003 instead of 2.0.3

reporter.newGauge(new MetricName("version", Collections.EMPTY_MAP), () -> (() -> sdkVersion));
reporter.start(1, TimeUnit.MINUTES);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd have hoped the metric can be added as just yet another metric that got exported rather than having a dedicated reporter with an hard-coded reporting period.

If that's registering a metric with a certain name, how about defining a MeterBinder ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@snicoll : The goal was to have a common place to report all internal sdk metrics. At Wavefront we use the prefix ~ to report internal metrics. I couldn't use a MeterBinder here as micrometer sanitizes the metric prefix ~sdk.* to _sdk.*.
Hence I created a WavefrontInternalReporter bean that could be used to report all internal diagnostic metrics for the SDK and as of now, ~sdk.java.wavefront_spring_boot_starter.version is the only internal metric.
Cc: @sushantdewan123 @hanwavefront

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I couldn't use a MeterBinder here as micrometer sanitizes the metric prefix ~sdk.* to _sdk.*.

If the naming convention normalization in Micrometer isn't correct for the Wavefront registry, we should fix that in Micrometer regardless of this change.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What Tommy said.

I don't think exposing such a bean with @ConditionalOnMissingBean is warranted. If we want to expose such internal metrics regardless of users customisations, the user should not have a say about it. I also question the fact there is a 1 minute hardcoded in there.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the naming convention normalization in Micrometer isn't correct for the Wavefront registry, we should fix that in Micrometer regardless of this change.

If someone lets me know what the correct set of allowed characters are, I can fix the sanitization in Micrometer. Or open an issue or pull request in Micrometer directly.
Though I suppose that would only allow this to work as expected with the next patch version of Micrometer or later.

Copy link
Collaborator

@snicoll snicoll Oct 13, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose that would only allow this to work as expected with the next patch version of Micrometer or later.

It's a new feature so I am totally fine with that. This one is parked for 2.1.0 anyway (which I expect us to build against Spring Boot 2.4.x).

return reporter;
}

@Configuration(proxyBeanMethods = false)
@ConditionalOnClass({ WavefrontMeterRegistry.class, MeterRegistryCustomizer.class })
Expand Down
1 change: 1 addition & 0 deletions ...ing-boot/src/main/java/com/wavefront/spring/autoconfigure/WavefrontSleuthSpanHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
import com.wavefront.java_sdk.com.google.common.collect.Sets;
import com.wavefront.sdk.common.NamedThreadFactory;
import com.wavefront.sdk.common.Pair;
import com.wavefront.sdk.common.Utils;
import com.wavefront.sdk.common.WavefrontSender;
import com.wavefront.sdk.common.application.ApplicationTags;
import com.wavefront.sdk.entities.tracing.SpanLog;
Expand Down
1 change: 1 addition & 0 deletions wavefront-spring-boot/src/main/resources/META-INF/wavefront-spring-boot/build.properties
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
project.version=${project.version}
43 changes: 43 additions & 0 deletions ...oot/src/test/java/com/wavefront/spring/autoconfigure/WavefrontAutoConfigurationTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,18 @@
package com.wavefront.spring.autoconfigure;

import java.lang.reflect.Field;
import java.util.Collections;
import java.util.Set;
import java.util.function.Function;
import java.util.function.Supplier;

import brave.Tracer;
import brave.TracingCustomizer;
import brave.handler.SpanHandler;

import com.wavefront.internal.reporter.WavefrontInternalReporter;
import com.wavefront.internal_reporter_java.io.dropwizard.metrics5.MetricName;
import com.wavefront.internal_reporter_java.io.dropwizard.metrics5.MetricRegistry;
import com.wavefront.opentracing.WavefrontTracer;
import com.wavefront.opentracing.reporting.Reporter;
import com.wavefront.sdk.appagent.jvm.reporter.WavefrontJvmReporter;
Expand All @@ -29,6 +35,7 @@
import org.springframework.cloud.sleuth.autoconfig.TraceAutoConfiguration;
import org.springframework.test.util.ReflectionTestUtils;

import static com.wavefront.spring.autoconfigure.WavefrontMetricsConfiguration.SDK_INTERNAL_METRIC_PREFIX;
import static org.assertj.core.api.Assertions.assertThat;
import static org.mockito.BDDMockito.given;
import static org.mockito.Mockito.mock;
Expand Down Expand Up @@ -136,6 +143,42 @@ void applicationTagsAreNotExportedToNonWavefrontRegistry() {
});
}

@Test
void internalReporterIsConfiguredWhenNoneExists() {
this.contextRunner
.with(wavefrontMetrics(() -> mock(WavefrontSender.class)))
.run((context) -> {
assertThat(context).hasSingleBean(WavefrontInternalReporter.class);
MetricRegistry internalMetricRegistry = (MetricRegistry)extractMetricRegistry(context
.getBean(WavefrontInternalReporter.class));
assertThat(internalMetricRegistry.getGauges().containsKey(new MetricName
(SDK_INTERNAL_METRIC_PREFIX + ".version", Collections.EMPTY_MAP)));
});
}

private Object extractMetricRegistry(WavefrontInternalReporter wavefrontInternalReporter) throws
NoSuchFieldException, IllegalAccessException {
Field field = wavefrontInternalReporter.getClass().getDeclaredField("internalRegistry");
field.setAccessible(true);
return field.get(wavefrontInternalReporter);
}

@Test
void internalReporterCanBeCustomized() {
WavefrontInternalReporter reporter = mock(WavefrontInternalReporter.class);
this.contextRunner
.with(wavefrontMetrics(() -> mock(WavefrontSender.class)))
.withBean(WavefrontInternalReporter.class, () -> reporter)
.run((context) -> assertThat(context).getBean(WavefrontInternalReporter.class).isEqualTo(reporter));
}

@Test
void internalReporterNotConfiguredWithoutWavefrontSender() {
this.contextRunner
.with(metrics())
.run(context -> assertThat(context).doesNotHaveBean(WavefrontInternalReporter.class));
}

akodali18 marked this conversation as resolved.
Show resolved Hide resolved
@Test
void jvmReporterIsConfiguredWhenNoneExists() {
this.contextRunner
Expand Down