chore: refactor otel tracing of Storage to be a decorator rather than in method modification #2856
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…g.c.storage.Storage methods Add introductory support for OpenTelemetry tracing to com.google.cloud.storage.Storage method. Actual RPC methods are not yet supported.
product-auto-label
bot
added
size: xl
BenWhitehead
commented
Jan 3, 2025
| } | ||
|
|
||
| SpanData spanData = exporter.getExportedSpans().get(0); | ||
| assertAll( |
There was a problem hiding this comment.
Make sure all are validated rather than an early failure preventing a later assertion
BenWhitehead
commented
Jan 3, 2025
| @After | ||
| public void tearDown() throws Exception { | ||
| if (storage != null) { | ||
| storage.close(); |
There was a problem hiding this comment.
make sure we close the constructed storage instance so we don't leak grpc clients
BenWhitehead
commented
Jan 3, 2025
| /** @since 2.47.0 This new api is in preview and is subject to breaking changes. */ | ||
| @BetaApi | ||
| public OpenTelemetry getDefaultOpenTelemetry() { | ||
| return OpenTelemetry.noop(); |
There was a problem hiding this comment.
Make sure we default to a non-null value
BenWhitehead
commented
Jan 3, 2025
| /** @since 2.47.0 This new api is in preview and is subject to breaking changes. */ | ||
| @BetaApi | ||
| public OpenTelemetry getDefaultOpenTelemetry() { | ||
| return OpenTelemetry.noop(); |
There was a problem hiding this comment.
Make sure we default to a non-null value
BenWhitehead
commented
Jan 3, 2025
| ResponseContentLifecycleManager.noop(), | ||
| grpcStorageOptions.blobWriteSessionConfig.createFactory(Clock.systemUTC()), | ||
| defaultOpts); | ||
| return OtelStorageDecorator.decorate( |
There was a problem hiding this comment.
Weave in the decorator if necessary
BenWhitehead
commented
Jan 3, 2025
| new StorageImpl( | ||
| httpStorageOptions, | ||
| httpStorageOptions.blobWriteSessionConfig.createFactory(clock)); | ||
| return OtelStorageDecorator.decorate( |
There was a problem hiding this comment.
Weave in the decorator if necessary
BenWhitehead
commented
Jan 3, 2025
Comment on lines
+76
to
+89
| Span span = | ||
| tracer | ||
| .spanBuilder("create") | ||
| .setAttribute("gsutil.uri", fmtBucket(bucketInfo.getName())) | ||
| .startSpan(); | ||
| try (Scope ignore = span.makeCurrent()) { | ||
| return delegate.create(bucketInfo, options); | ||
| } catch (Throwable t) { | ||
| span.recordException(t); | ||
| span.setStatus(StatusCode.ERROR, t.getClass().getSimpleName()); | ||
| throw t; | ||
| } finally { | ||
| span.end(); | ||
| } |
There was a problem hiding this comment.
This class is the largest chunk of the PR, but consists of generally templated code.
- Define a span and scope
- Invoke the delegate
- catch any error
- end the span
BenWhitehead
commented
Jan 3, 2025
| } | ||
| } | ||
|
|
||
| private static class OtelDecoratedReadChannel implements ReadChannel { |
There was a problem hiding this comment.
ReadChannel, WriteChannel, CopyWriter and BlobWriteSession have sub-decorators which carry their originating spans along for their lifetimes.
sydney-munro
approved these changes
Jan 6, 2025
sydney-munro
added
the
kokoro:force-run
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Refactor com.google.cloud.storage.Storage otel tracing to be based on a decorating instance of Storage rather than modifying the implementations of StorageImpl and GrpcStorageImpl directly.
This simplifies things by ensuring any otel tracing for c.g.c.s.Storage is implemented for both http and grpc at the same time and with the same metadata.
As a side effect, the previous com.google.cloud.storage.otel package has been removed.
The number of lines appears daunting, however a high level overview of the actual modifications:
@BetaApion all otel related methods{Http,Grpc}StorageDefaults.getDefaultOpenTelemetrydefault which points atOpenTelemetry.noop()com.google.cloud.storage.otelpackage and helpersassertAll