feat: enable open telemetry metrics for GRPC #2590
Conversation
product-auto-label
bot
added
size: l
JesseLovelace
added
the
do not merge
google-cloud-storage/pom.xml
Outdated
| <!-- Open Telemetry dependencies --> | ||
| <dependency> | ||
| <groupId>io.opentelemetry</groupId> | ||
| <artifactId>opentelemetry-sdk</artifactId> | ||
| <version>1.37.0</version> | ||
| </dependency> | ||
| <dependency> | ||
| <groupId>io.grpc</groupId> | ||
| <artifactId>grpc-opentelemetry</artifactId> | ||
| <version>1.64.0</version> | ||
| </dependency> | ||
| <dependency> | ||
| <groupId>io.opentelemetry</groupId> | ||
| <artifactId>opentelemetry-api</artifactId> | ||
| <version>1.37.0</version> | ||
| </dependency> | ||
|
|
||
| <dependency> | ||
| <groupId>io.opentelemetry</groupId> | ||
| <artifactId>opentelemetry-sdk-metrics</artifactId> | ||
| <version>1.37.0</version> | ||
| </dependency> | ||
| <dependency> | ||
| <groupId>io.opentelemetry</groupId> | ||
| <artifactId>opentelemetry-sdk-common</artifactId> | ||
| <version>1.37.0</version> | ||
| </dependency> | ||
| <dependency> | ||
| <groupId>io.opentelemetry</groupId> | ||
| <artifactId>opentelemetry-sdk-extension-autoconfigure</artifactId> | ||
| <version>1.37.0</version> | ||
| </dependency> | ||
|
|
||
| <dependency> | ||
| <groupId>com.google.cloud.opentelemetry</groupId> | ||
| <artifactId>detector-resources</artifactId> | ||
| <version>0.27.0-alpha</version> | ||
| </dependency> | ||
| <dependency> | ||
| <groupId>com.google.cloud.opentelemetry</groupId> | ||
| <artifactId>exporter-metrics</artifactId> | ||
| <version>0.29.0</version> | ||
| </dependency> | ||
| <dependency> | ||
| <groupId>io.opentelemetry.instrumentation</groupId> | ||
| <artifactId>opentelemetry-resources</artifactId> | ||
| <version>2.3.0-alpha</version> | ||
| </dependency> | ||
| <dependency> | ||
| <groupId>io.opentelemetry.contrib</groupId> | ||
| <artifactId>opentelemetry-gcp-resources</artifactId> | ||
| <version>1.34.0-alpha</version> | ||
| </dependency> |
There was a problem hiding this comment.
Can you add all these open telemetry related dependencies into a group in the renovate config so we only get a single PR for all of them rather than a dozen? https://github.com/googleapis/java-storage/blob/main/renovate.json
There was a problem hiding this comment.
Ideally all of the otel version come from shared dependencies. If we can't get them from shared-deps we'll need the renovate config.
google-cloud-storage/pom.xml
Outdated
| <dependencyManagement> | ||
| <dependencies> | ||
| <dependency> | ||
| <groupId>io.opentelemetry.semconv</groupId> | ||
| <artifactId>opentelemetry-semconv</artifactId> | ||
| <version>1.25.0-alpha</version> | ||
| </dependency> | ||
| </dependencies> | ||
| </dependencyManagement> | ||
|
|
There was a problem hiding this comment.
This isn't listed as an actual dependency, is this needed?
There was a problem hiding this comment.
I was getting a build error without this, something about parent dependencies not resolving to the same version
There was a problem hiding this comment.
I was only getting that error when trying to make a shaded jar though, so maybe it's not strictly necessary
There was a problem hiding this comment.
hmm, that's somewhat concerning. If you get the error again, lets dig into it and see if we can suss out the conflict.
| @@ -59,7 +59,7 @@ | |||
|
|
|||
| @RunWith(StorageITRunner.class) | |||
| @CrossRun( | |||
| backends = {Backend.TEST_BENCH}, | |||
| backends = {Backend.PROD}, | |||
There was a problem hiding this comment.
Can this be reverted? Looks to be a left over.
google-cloud-storage/src/test/java/com/google/cloud/storage/ITGrpcMetricsTest.java
Outdated
Show resolved
Hide resolved
google-cloud-storage/src/test/java/com/google/cloud/storage/ITGrpcMetricsTest.java
Show resolved
Hide resolved
| @@ -337,6 +366,215 @@ private Tuple<StorageSettings, Opts<UserProject>> resolveSettingsAndOpts() throw | |||
| return Tuple.of(builder.build(), defaultOpts); | |||
| } | |||
|
|
|||
| private void enableGrpcMetrics( | |||
There was a problem hiding this comment.
Can we move all these helpers into a new class (OpenTelemetryBootstrappingUtils.java perhaps) and make them static?
| GrpcOpenTelemetry.newBuilder() | ||
| .sdk(openTelemetrySdk) | ||
| .enableMetrics( | ||
| Arrays.asList( |
There was a problem hiding this comment.
| Arrays.asList( | |
| ImmutableList.of( |
| <dependency> | ||
| <groupId>com.google.cloud.opentelemetry</groupId> | ||
| <artifactId>exporter-metrics</artifactId> | ||
| <version>0.29.0</version> | ||
| </dependency> |
There was a problem hiding this comment.
This artifact appears to be java11+
❯ javapMavenJars.bash com.google.cloud.opentelemetry:exporter-metrics
2024-06-18 15:40:48 com.google.cloud.opentelemetry:exporter-metrics:0.29.0
2024-06-18 15:40:48 major version: 55 // java11
Can you try to find who the maintainer is in Google and ask them if this is intentional?
The sibling artifact is java8+
❯ javapMavenJars.bash com.google.cloud.opentelemetry:detector-resources
2024-06-18 15:51:25 com.google.cloud.opentelemetry:detector-resources:0.27.0-alpha
2024-06-18 15:51:26 major version: 52 // java8
Co-authored-by: BenWhitehead <BenWhitehead@users.noreply.github.com>
|
Warning: This pull request is touching the following templated files:
|
Enables open telemetry metrics for GRPC
Note: Currently, this emits the metrics with a "generic_task" mapping. In the future, there will be a "gcs_client" mapping supported, with more relevant attributes. We'll need to switch to that when it's available, so I've left in comments to uncomment when we make that switch