Skip to content

Logs in logger#282

Open
brianquinlan wants to merge 5 commits into
googleapis:mainfrom
brianquinlan:logs_in_logger
Open

Logs in logger#282
brianquinlan wants to merge 5 commits into
googleapis:mainfrom
brianquinlan:logs_in_logger

Conversation

@brianquinlan

Copy link
Copy Markdown
Contributor

No description provided.

@gemini-code-assist gemini-code-assist Bot left a comment

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.

Code Review

This pull request introduces W3C traceparent support for structured logging, enabling automatic trace and span ID correlation by extracting data from the execution Zone. Key changes include a new traceparent parser, updates to the structured logger and shelf middleware, and the addition of comprehensive E2E and unit tests. Review feedback highlights critical issues with unreleased dependency versions in the pubspec file and suggests refining the E2E test server logic to avoid unsupported trace header formats.

Comment thread pkgs/google_cloud_logging/pubspec.yaml Outdated
Comment on lines +21 to +24
googleapis_auth: ^2.0.0
http: ^1.3.0
test: ^1.31.0
test_utils: any

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.

critical

The version constraints for googleapis_auth and http appear to be for unreleased versions and will likely cause dart pub get to fail. Please update these to valid and stable version constraints.

  googleapis_auth: ^1.6.0
  http: ^1.2.1
  test: ^1.31.0
References
  1. A wide dependency version range is acceptable if compatibility across the entire range has been verified.

Comment on lines +29 to +31
final traceparent =
request.headers.value('traceparent') ??
request.headers.value('x-cloud-trace-context');

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.

medium

This logic falls back to using the x-cloud-trace-context header if traceparent is not present. However, the CloudLogger.structuredLogger() used in this test server is configured to parse only the W3C traceparent format, not the x-cloud-trace-context format. If an x-cloud-trace-context header is received, its value will be passed to the logger, which will fail to parse it, and no trace correlation will occur. To avoid confusion, it would be clearer to only handle the traceparent header.

Suggested change
final traceparent =
request.headers.value('traceparent') ??
request.headers.value('x-cloud-trace-context');
final traceparent =
request.headers.value('traceparent');

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant