googleapis / nodejs-logging-bunyan

Node.js client integration between Stackdriver Logging and Bunyan.
https://cloud.google.com/logging/
Apache License 2.0
63 stars 34 forks source link

fix: skip parent request entry on cloud run #658

Closed wvanderdeijl closed 2 years ago

wvanderdeijl commented 2 years ago

Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:

Fixes #626

We also suffer from this issue. As a (dirty) workaround we have no added the following code to our own codebase:

// workaround for https://github.com/googleapis/nodejs-logging-bunyan/issues/626 to let lib know we are running in GCP
if (process.env.K_SERVICE && !process.env.GAE_SERVICE) {
    // trick library into thinking we run on AppEngine
    process.env.GAE_SERVICE = process.env.K_SERVICE;
}
generated-files-bot[bot] commented 2 years ago

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

wvanderdeijl commented 2 years ago

You mean to test this behaviour in system-test/test-middleware-express.ts? But that test currently doesn't cover anything about the parent request entry. I could add assertions there for the additional parent request entry but that would make the test dependent on whether you are running in GCP or not as the behaviour of the test would be different. What do you want me to do?

losalex commented 2 years ago

nodejs-logging-bunyan/test/middleware/test-express.ts

I believe we should make sure that for GCPEnv.CLOUD_RUN the behavior is correct when skipParentEntryForCloudRun is set to true

wvanderdeijl commented 2 years ago

@losalex isnt’t that the test I added to nodejs-logging-bunyan/test/middleware/test-express.ts? Or are you looking for something else?

losalex commented 2 years ago

@losalex isnt’t that the test I added to nodejs-logging-bunyan/test/middleware/test-express.ts? Or are you looking for something else?

@wvanderdeijl , my bad, it seems I might confuse you - would you be able to merge your new test with one above? This way we can keep them all together to make sure it will prevent any confusion in the future. Approving the request anyway, up to you if you can make a change to unify the test - good job and thank you!

wvanderdeijl commented 2 years ago

@losalex I've integrated into a single test as you requested. But this removed your approval and the work from the bot that updated the README