-
Notifications
You must be signed in to change notification settings - Fork 873
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add metric support for grpc #5923
Conversation
@@ -44,7 +46,7 @@ public final class LibraryTestRunner extends InstrumentationTestRunner { | |||
GlobalOpenTelemetry.resetForTest(); | |||
|
|||
testSpanExporter = InMemorySpanExporter.create(); | |||
testMetricExporter = InMemoryMetricExporter.create(); | |||
testMetricExporter = InMemoryMetricExporter.create(AggregationTemporality.DELTA); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cumulative metrics are a bad fit for tests. We have a shard instance of the SDK for all tests. If temporality is cumulative, then metrics that initially received measurements for one test will continue to report for the remaining tests. The result is very cumbersome to assert against and debug.
...ting/src/main/java/io/opentelemetry/instrumentation/grpc/v1_6/AbstractGrpcStreamingTest.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
📈
@@ -34,7 +34,7 @@ void reset() { | |||
|
|||
@Override | |||
public AggregationTemporality getAggregationTemporality(InstrumentType instrumentType) { | |||
return AggregationTemporality.CUMULATIVE; | |||
return AggregationTemporality.DELTA; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jack-berg this seems to make a micrometer test a bit flaky https://ge.opentelemetry.io/s/mwt7qlr5nwqig/tests/:instrumentation:micrometer:micrometer-1.5:javaagent:testBaseTimeUnit/io.opentelemetry.javaagent.instrumentation.micrometer.v1_5.TimerSecondsTest/testTimerWithBaseUnitSeconds()?expanded-stacktrace=WyIwIl0&top-execution=1
To reproduce insert Thread.sleep(1_000);
between timer.record()
calls. Any ideas where the issue is?
* Add metric support for grpc * Spotless
Was surprised to learn that gRPC metrics weren't already enabled. Any reason not to enable them?