Add build properties file in its own namespace to carry project versi… #78
Conversation
There was a problem hiding this comment.
I am not a huge fan of shipping build.properties this way and would have preferred to use the manifest as I've explained in another issue. However, it looks like that ship has sailed.
...spring-boot/src/main/java/com/wavefront/spring/autoconfigure/WavefrontSleuthSpanHandler.java
Outdated
Show resolved
Hide resolved
…version metric to be part of auto configuration.
akodali18
force-pushed
the
build-property
branch
from
2d80143 to
84a2092
Compare
There was a problem hiding this comment.
Thank you for the update. The internal reporter makes me a bit nervous and I wonder if there isn't a less invasive way to report that information.
Also paging @shakuzen if he has recommendations about exporting such information.
...ing-boot/src/main/java/com/wavefront/spring/autoconfigure/WavefrontMetricsConfiguration.java
Outdated
Show resolved
Hide resolved
...ing-boot/src/main/java/com/wavefront/spring/autoconfigure/WavefrontMetricsConfiguration.java
Show resolved
Hide resolved
| build(wavefrontSender); | ||
| Double sdkVersion = Utils.getSemVerGauge("wavefront-spring-boot"); | ||
| reporter.newGauge(new MetricName("version", Collections.EMPTY_MAP), () -> (() -> sdkVersion)); | ||
| reporter.start(1, TimeUnit.MINUTES); |
There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
@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
There was a problem hiding this comment.
I couldn't use a
MeterBinderhere 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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
...g-boot/src/test/java/com/wavefront/spring/autoconfigure/WavefrontAutoConfigurationTests.java
Show resolved
Hide resolved
| WavefrontInternalReporter reporter = new WavefrontInternalReporter.Builder(). | ||
| prefixedWith(SDK_INTERNAL_METRIC_PREFIX).withSource(wavefrontConfig.source()). | ||
| build(wavefrontSender); | ||
| Double sdkVersion = Utils.getSemVerGauge("wavefront-spring-boot"); |
There was a problem hiding this comment.
How is, for example, the current version 2.0.1 represented as a Double?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
|
I've removed this one for |
…on info.