FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

feat: Add OpenTelemetry Traces to GRPC by sydney-munro · Pull Request #2783 · googleapis/java-storage · GitHub

/ java-storage Public

feat: Add OpenTelemetry Traces to GRPC - #2783

Merged
sydney-munro merged 6 commits into
otel-v1-branchfrom
otel-grpc
Oct 22, 2024
ConversationCommits (6)ChecksFiles changed
Merged

feat: Add OpenTelemetry Traces to GRPC#2783
sydney-munro merged 6 commits into
otel-v1-branchfrom
otel-grpc

Conversation

sydney-munro commented Oct 18, 2024

Copy link
Copy Markdown
Contributor

To keep this more closely aligned with the introduction in HTTP the scope here is small.

To follow will be to add the attributes for recording exceptions and testing that flow.

sydney-munro requested review from a team and frankyn October 18, 2024 18:35
product-auto-label Bot added size: m Pull request size is medium. api: storage Issues related to the googleapis/java-storage API. labels Oct 18, 2024
sydney-munro requested a review from a team October 22, 2024 15:27

generated-files-bot Bot commented Oct 22, 2024

Copy link
Copy Markdown

Warning: This pull request is touching the following templated files:

  • .github/sync-repo-settings.yaml

JesseLovelace reviewed Oct 22, 2024
View reviewed changes
Comment thread google-cloud-storage/src/main/java/com/google/cloud/storage/GrpcStorageOptions.java Outdated
/**
* Enable OpenTelemetry Tracing and provide an instance for the client to use.
*
* @param openTelemetrySdk

JesseLovelace Oct 22, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I think we need a description of the param here, or just delete the @PARAM line

sydney-munro Oct 22, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

done. added a description.

sydney-munro merged commit 5e6eb84 into otel-v1-branch Oct 22, 2024
sydney-munro deleted the otel-grpc branch October 22, 2024 16:34
frankyn reviewed Oct 22, 2024
View reviewed changes
Comment thread google-cloud-storage/src/test/java/com/google/cloud/storage/ITOpenTelemetryTest.java
// NoOp for Grpc
StorageOptions storageOptionsGrpc = StorageOptions.grpc().build();
Storage storageGrpc = storageOptionsGrpc.getService();
storageGrpc.create(BucketInfo.of(grpcBucket));

frankyn Oct 22, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

What is no-op testing and checking?

sydney-munro Oct 22, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

just checking to make sure that it still runs without issue and does nothing. basically just that it doesnt error out.

Comment thread .github/sync-repo-settings.yaml
- clirr
- units (8)
- units (11)
- 'Kokoro - Test: Integration'

frankyn Oct 22, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Does Kokoro need to be updated internally to watch this branch as well?

sydney-munro Oct 22, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

ill double check!

Comment thread google-cloud-storage/src/test/java/com/google/cloud/storage/ITOpenTelemetryTest.java
String bucket = randomBucketName();
storage.create(BucketInfo.of(bucket));
TestExporter testExported = (TestExporter) exporter;
SpanData spanData = testExported.getExportedSpans().get(0);

frankyn Oct 22, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Do you plan on testing which spans are collected per operation in addition to Span attributes? I think this would be more useful for Upload and Download ops.

sydney-munro Oct 22, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I don't know if i understand this question. what do you mean by which spans are collected

frankyn Oct 22, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Oh i meant; let's say you're uploading a large object to GCS. I would assume there's a span for CreateResumableUpload, then span for each WriteObject request. Wondering if there's a way to test that using spanData.

sydney-munro Oct 22, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

ah I see, I haven't expanded this for that case yet but yeah we would want to test each span is generated. Maybe we could check for the size to make sure if we expect 3 writeObjectRequests we would have 3 spans

frankyn Oct 22, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

It's a good start to assert span count; and expand from there.

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
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers
frankyn frankyn left review comments
JesseLovelace JesseLovelace approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

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

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

Back | FazBrowse Home | New Git URL