Micrometer instrumentation, part 2 #5001
Conversation
mateuszrzeszutek
requested review from
anuraaga,
iNikem,
jkwatson,
laurit,
pavolloffay,
trask and
tylerbenson
as code owners
| new OpenTelemetryDistributionSummary( | ||
| id, clock, distributionStatisticConfig, scale, otelMeter); | ||
| if (distributionSummary.isUsingMicrometerHistograms()) { | ||
| HistogramGauges.registerWithCommonFormat(distributionSummary, this); |
There was a problem hiding this comment.
ah, this is the magic that registers the internal gauges back with our meter registry
...java/io/opentelemetry/javaagent/instrumentation/micrometer/v1_5/AsyncInstrumentRegistry.java
Show resolved
Hide resolved
| countMeterName = id.getName() + "." + Statistic.COUNT.getTagValueRepresentation(); | ||
| totalTimeMeterName = id.getName() + "." + Statistic.TOTAL_TIME.getTagValueRepresentation(); | ||
|
|
||
| asyncInstrumentRegistry.buildLongCounter( |
There was a problem hiding this comment.
@jack-berg I think this is a good use case for an async batch API
There was a problem hiding this comment.
What advantage do you seek from a batch API vs the existing API here?
| } | ||
|
|
||
| void removeDoubleCounter(String name, Attributes attributes) { | ||
| synchronized (doubleCounters) { |
There was a problem hiding this comment.
#nit: can move the synchronization to inside removeMeasurement to DRY up the code a bit.
There was a problem hiding this comment.
Unfortunately errorprone doesn't allow to compile it (because of @GuardedBy), so I have to duplicate this synchronized block.
| @Override | ||
| public double max() { | ||
| UnsupportedReadLogger.logWarning(); | ||
| return Double.NaN; |
There was a problem hiding this comment.
Do you want to return Double.NaN here or 0?
There was a problem hiding this comment.
I think I would prefer returning NaN everywhere - since it clearly implies that the value returned by this method should not ever be used.
| @Override | ||
| public double totalTime(TimeUnit unit) { | ||
| UnsupportedReadLogger.logWarning(); | ||
| return Double.NaN; |
There was a problem hiding this comment.
See #5001 (comment)
...a/io/opentelemetry/javaagent/instrumentation/micrometer/v1_5/OpenTelemetryFunctionTimer.java
Outdated
Show resolved
Hide resolved
mateuszrzeszutek
force-pushed
the
micrometer-2
branch
from
bbcd54d to
94cdb9a
Compare
|
Merged that PR to unblock the next one (since there's a couple of approvals here); if you have any further comments, don't hesitate to add them here - I can address them in later PRs. |
* Micrometer instrumentation, part 2 * use double counter instead of gauge in FunctionTimer * code review suggestions * fix errorprone * rebase and fix compilation failure
Continuation of #4919
This PR contains the implementations of following micrometer instruments:
DistributionSummaryFunctionCounterFunctionTimerLongTaskTimerand customMeterare still missing, they'll be added in the next PR.