Skip to content
This repository was archived by the owner on Feb 7, 2026. It is now read-only.

fix: skip parent request entry on cloud run#658

Merged
losalex merged 2 commits intogoogleapis:mainfrom
wvanderdeijl:fix-626
Oct 10, 2022
Merged

fix: skip parent request entry on cloud run#658
losalex merged 2 commits intogoogleapis:mainfrom
wvanderdeijl:fix-626

Conversation

@wvanderdeijl
Copy link
Copy Markdown
Contributor

@wvanderdeijl wvanderdeijl commented Oct 5, 2022

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:

  • Make sure to open an issue as a bug/issue before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea
  • Ensure the tests and linter pass
  • Code coverage does not decrease (if any source code was changed)
  • Appropriate docs were updated (if necessary)

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;
}

@wvanderdeijl wvanderdeijl requested review from a team October 5, 2022 10:40
@product-auto-label product-auto-label Bot added the size: s Pull request size is small. label Oct 5, 2022
@product-auto-label product-auto-label Bot added the api: logging Issues related to the googleapis/nodejs-logging-bunyan API. label Oct 5, 2022
@generated-files-bot
Copy link
Copy Markdown

generated-files-bot Bot commented Oct 5, 2022

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

@wvanderdeijl wvanderdeijl force-pushed the fix-626 branch 2 times, most recently from 29a0cb3 to 5cc6c99 Compare October 5, 2022 10:50
@losalex losalex assigned losalex and unassigned minherz Oct 5, 2022
losalex
losalex reviewed Oct 5, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@losalex losalex left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks @wvanderdeijl for making this change! Overall the change looks good. I wonder if you can also cover this behavior in system-test like one we have in:

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

Line 128 in f3ed3aa

[GCPEnv.APP_ENGINE, GCPEnv.CLOUD_FUNCTIONS].forEach(env => {

@wvanderdeijl
Copy link
Copy Markdown
Contributor Author

wvanderdeijl commented Oct 6, 2022 •
edited
Loading

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 losalex added the owlbot:run Add this label to trigger the Owlbot post processor. label Oct 6, 2022
@gcf-owl-bot gcf-owl-bot Bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Oct 6, 2022
@losalex
Copy link
Copy Markdown
Contributor

losalex commented Oct 6, 2022

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
Copy link
Copy Markdown
Contributor Author

wvanderdeijl commented Oct 6, 2022

@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
Copy link
Copy Markdown
Contributor

losalex commented Oct 6, 2022 •
edited
Loading

@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!

@losalex losalex changed the title skip parent request entry on cloud run fix: skip parent request entry on cloud run Oct 7, 2022
@losalex losalex added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Oct 7, 2022
losalex
losalex approved these changes Oct 7, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@losalex losalex left a comment

Choose a reason for hiding this comment

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

LGTM

@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Oct 7, 2022
@wvanderdeijl
Copy link
Copy Markdown
Contributor Author

wvanderdeijl commented Oct 10, 2022

@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

@losalex losalex added the owlbot:run Add this label to trigger the Owlbot post processor. label Oct 10, 2022
@gcf-owl-bot gcf-owl-bot Bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Oct 10, 2022
@losalex losalex added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Oct 10, 2022
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Oct 10, 2022
@losalex losalex merged commit 226972e into googleapis:main Oct 10, 2022
@release-please release-please Bot mentioned this pull request Oct 10, 2022
Merged
@wvanderdeijl wvanderdeijl deleted the fix-626 branch October 22, 2022 19:55
@yuku yuku mentioned this pull request May 27, 2023
Closed
@cindy-peng cindy-peng mentioned this pull request Oct 20, 2023
Merged
4 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@losalex losalex losalex approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@losalex losalex

Labels

api: logging Issues related to the googleapis/nodejs-logging-bunyan API. size: s Pull request size is small.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Duplicate http request entries when running in Cloud Run

4 participants

@wvanderdeijl @losalex @minherz @yoshi-kokoro