Metrics: Synchronous gauges respect attributes #101018
Conversation
Synchronous gauges would only accept the most recent value, regardless of matching attributes. As attributes should be treated as dimensions, we keep the most recent value with matching attributes.
elasticsearchmachine
added
v8.12.0
Team:Core/Infra
|
Pinging @elastic/es-core-infra (Team:Core/Infra) |
|
Hi @stu-elastic, I've created a changelog YAML for you. |
There was a problem hiding this comment.
I left some comments
at some point apm-java-agent should provide us with synchronous gague implementation, so perhaps we don't need to worry about this too much open-telemetry/opentelemetry-java#5506
@JonasKunz do you know when we could use it? once it stop being experimental?
...apm/src/main/java/org/elasticsearch/telemetry/apm/internal/metrics/AbstractGaugeAdapter.java
Show resolved
Hide resolved
| } | ||
|
|
||
| // testing that a value reported is then used in a callback | ||
| @SuppressWarnings("unchecked") |
There was a problem hiding this comment.
i think we can remove this now
There was a problem hiding this comment.
It would be great to remove it, since we no longer have an unchecked cast here
| LongGaugeAdapter gauge1 = new LongGaugeAdapter(meter, "name1", "desc", "unit"); | ||
| LongGaugeAdapter gauge2 = new LongGaugeAdapter(meter, "name2", "desc", "unit"); | ||
| Map<String, Object> map = Map.of("k", 1L); | ||
| gauge1.record(1L, map); |
There was a problem hiding this comment.
why is this test called same values?
There was a problem hiding this comment.
Ahh it should be same attributes.
There was a problem hiding this comment.
should we then rename the test?
| * Side Public License, v 1. | ||
| */ | ||
|
|
||
| package org.elasticsearch.telemetry.apm.internal.metrics; |
There was a problem hiding this comment.
should we not have test framework classes in separate PR?
on the other side we have some example usage of it in this PR, so maybe it is fine
There was a problem hiding this comment.
The full framework is in #100977 but I'm having some dependency issues. I didn't want to keep blocking progress on gauges on that.
| } | ||
| } | ||
|
|
||
| private final Map<INSTRUMENT, RegisteredMetric> doubles; |
There was a problem hiding this comment.
i wonder if Instrument would not read better
I was looking for a generic INSTRUMENT here because of capital casing
There was a problem hiding this comment.
just a ping, that it would be great to rename this before merging
| } | ||
|
|
||
| // testing that a value reported is then used in a callback | ||
| @SuppressWarnings("unchecked") |
There was a problem hiding this comment.
It would be great to remove it, since we no longer have an unchecked cast here
| LongGaugeAdapter gauge1 = new LongGaugeAdapter(meter, "name1", "desc", "unit"); | ||
| LongGaugeAdapter gauge2 = new LongGaugeAdapter(meter, "name2", "desc", "unit"); | ||
| Map<String, Object> map = Map.of("k", 1L); | ||
| gauge1.record(1L, map); |
There was a problem hiding this comment.
should we then rename the test?
| } | ||
| } | ||
|
|
||
| private final Map<INSTRUMENT, RegisteredMetric> doubles; |
There was a problem hiding this comment.
just a ping, that it would be great to rename this before merging
| assertThat(testLongMeasurement.attributes, Matchers.equalTo(Attributes.builder().put("k", 1).build())); | ||
| meter.collectMetrics(); | ||
| metrics = meter.getRecorder().get(gauge); | ||
| assertThat(metrics, hasSize(2)); |
There was a problem hiding this comment.
if you check containsInAnyOrder you don't need to check for a size. It would fail if an actual collection has less,more elements then expected matchers
| return recorder; | ||
| } | ||
|
|
||
| private MeterRecorder recorder = new MeterRecorder(); |
There was a problem hiding this comment.
can we fix the order? fields first, methods later.
might be worth being concistent with field accessors
|
|
||
| package org.elasticsearch.telemetry.apm.internal.metrics; | ||
|
|
||
| import io.opentelemetry.api.common.Attributes; |
There was a problem hiding this comment.
do we really have to implement an otel api in test framework?
can we not limit ourselfs to just implementing the recording version of the org.elasticsearch.telemetry.metric.Meter?
Synchronous gauges would only accept the most recent value, regardless of matching attributes. As attributes should be treated as dimensions, we keep the most recent value with matching attributes.