feat: Instrument HTTP Reads and Rewrites - #2808
Conversation
Sorry, something went wrong.
| @Override | ||
| public long read( | ||
| StorageObject from, Map<Option, ?> options, long position, OutputStream outputStream) { | ||
| OpenTelemetryTraceUtil.Span otelSpan = openTelemetryTraceUtil.startSpan("read"); |
There was a problem hiding this comment.
I misunderstood the span name; Does startSpan provide context on class for this method?
Pattern: $package.$service/$method
Example using C++: storage::Client::WriteObject/CreateResumableUpload
Sorry, something went wrong.
There was a problem hiding this comment.
Sorry, something went wrong.
There was a problem hiding this comment.
Oh, I was thinking it would be
storage.spi.v1/read in this case so it's clear where this span exists.
@cojenco could you take a look at this?
Sorry, something went wrong.
There was a problem hiding this comment.
+1 to providing context on the class/module for this method, especially considering there are many call paths in java
The OTel semantic conventions recommends the pattern of $package.$service/$method where $method MUST NOT contain slashes. I'm not very familiar with how the classes are structured here, but I agree that it would be a good idea to include some context of spi/v1/HttpStorageRpc in the span name for better debuggability
Sorry, something went wrong.
frankyn
left a comment
There was a problem hiding this comment.
We can always update span names after.
Sorry, something went wrong.
Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:
Fixes #<issue_number_goes_here> ☕️
If you write sample code, please follow the samples format.