-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
feat(logging): OpenTelemetry trace/span ID integration for Go logging library #10030
Conversation
There was a problem hiding this 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) { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
ctx := context.Background() | ||
|
||
// Set up an OTel SDK tracer if integration test, mock noop tracer if not. | ||
if integrationTest { |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
Fixes #9302