Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(logging): OpenTelemetry trace/span ID integration for Go logging library #10030

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

gkevinzheng
Copy link
Contributor

Fixes #9302

@gkevinzheng gkevinzheng requested review from a team as code owners April 23, 2024 20:48
@product-auto-label product-auto-label bot added the api: logging Issues related to the Cloud Logging API. label Apr 23, 2024
Copy link
Contributor

@daniel-sanche daniel-sanche left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -609,6 +611,170 @@ func TestToLogEntry(t *testing.T) {
}
}

func TestToLogEntryOTelIntegration(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we plan to add system tests to start otel span and compare auto-generated otel spanId with the injected logEntry spanId?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cindy-peng I just added an integration/system test for the exact same test case, just with different span setups.

@gkevinzheng gkevinzheng added the kokoro:run Add this label to force Kokoro to re-run the tests. label May 3, 2024
@kokoro-team kokoro-team removed the kokoro:run Add this label to force Kokoro to re-run the tests. label May 7, 2024
ctx := context.Background()

// Set up an OTel SDK tracer if integration test, mock noop tracer if not.
if integrationTest {
Copy link
Contributor

@cindy-peng cindy-peng May 7, 2024

Choose a reason for hiding this comment

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

Do we have a dedicated place for integration tests in Go Logging library?
For example, for Java-logging and Node-logging, there is a separate folder for integration tests:
https://github.com/googleapis/java-logging/tree/main/google-cloud-logging/src/test/java/com/google/cloud/logging/it
https://github.com/googleapis/nodejs-logging/tree/main/system-test

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think we have any right now, but if enough system tests happen that are kinda different from integration tests controlled by the integrationTest flag I could make a separate file for them.

@gkevinzheng gkevinzheng requested a review from dashpole May 7, 2024 16:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: logging Issues related to the Cloud Logging API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

logging: OpenTelemetry spans are ignored as parents for LogRecords
6 participants