-
Notifications
You must be signed in to change notification settings - Fork 438
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): add trace to logging on cloud run #6764
feat(logging): add trace to logging on cloud run #6764
Conversation
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
Hi guys, I made this PR getting code from main branch, so I don't understand why Backwards compatibility check is failing. What am I supposed to do to fix this? Thanks |
Hi @javihgil , thanks a lot for taking time and contributing to the library.
It was caused due to some merge conflicts getting resolved in a way which deleted incoming changes. I've fixed it and now all the tests pass. Can you please tell me your inspiration behind this PR? I would like to know what were you trying to do which led you to finding this feature absence and how doing this would help your work? |
Of course! When you use this library with GAE, the Google\Cloud\Core\Report\GAEMetadataProvider sets an "appengine.googleapis.com/trace_id" label witch allows you to filter log entries by this trace, whose value is provided by HTTP_X_CLOUD_TRACE_CONTEXT header. With CloudRun this header is also received, but it's ignored by CloudRunMetadataProvider so you can not filter log entries by trace_id. This PR adds this label in metadata provider. I renamed it to "run.googleapis.com/trace_id" instead of using "appengine.googleapis.com/trace_id" because I think is the propper way. Thanks! |
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.
Hi @javihgil , sorry for the long delay in this PR review.
So I've cross checked that we are getting the HTTP_X_CLOUD_TRACE_CONTEXT
env variable set by the cloud run env. It's of the format specified here. Also, the naming of the label seems fine to me as it's inline with the cloud run's url . This PR seems fine to me with just one nit : )
@bshaffer , Can you please take look if everything looks okay from your side?
Ref: #1407 and b/312685152
Update the `PsrLoggerBatchTest:: testTraceIdLabelOnGAEFlex` to `PsrLoggerBatchTest:: testTraceIdLabelOnServerlessPlatforms` as it now tests GAE as well as Cloud Run.
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
@@ -5,7 +5,7 @@ | |||
"minimum-stability": "stable", | |||
"require": { | |||
"php": "^8.0", | |||
"google/cloud-core": "^1.52.7", | |||
"google/cloud-core": "^1.58", |
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.
Bump the google/cloud-core
dependency to what will be the next release, as the tests depend on the new metadata provider class.
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! Thank you for adding this.
No description provided.