KEMBAR78
feat: Instrument HTTP Reads and Rewrites by sydney-munro · Pull Request #2808 · googleapis/java-storage · GitHub
Skip to content

Conversation

@sydney-munro
Copy link
Contributor

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:

  • Make sure to open an issue as a bug/issue before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea
  • Ensure the tests and linter pass
  • Code coverage does not decrease (if any source code was changed)
  • Appropriate docs were updated (if necessary)

Fixes #<issue_number_goes_here> ☕️

If you write sample code, please follow the samples format.

@sydney-munro sydney-munro requested a review from a team as a code owner November 4, 2024 23:53
@product-auto-label product-auto-label bot added size: m Pull request size is medium. api: storage Issues related to the googleapis/java-storage API. labels Nov 4, 2024
@Override
public long read(
StorageObject from, Map<Option, ?> options, long position, OutputStream outputStream) {
OpenTelemetryTraceUtil.Span otelSpan = openTelemetryTraceUtil.startSpan("read");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+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

Copy link
Contributor

@frankyn frankyn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can always update span names after.

@sydney-munro sydney-munro merged commit 4538213 into otel-v1-branch Nov 7, 2024
17 checks passed
@sydney-munro sydney-munro deleted the http-instrument branch November 7, 2024 22:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

api: storage Issues related to the googleapis/java-storage API. size: m Pull request size is medium.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants