Skip to content
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

fix: skip parent request entry on cloud run #658

Merged
merged 2 commits into from
Oct 10, 2022

Conversation

wvanderdeijl
Copy link
Contributor

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 as code owners 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

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
Copy link
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.

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:

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

@wvanderdeijl
Copy link
Contributor Author

wvanderdeijl commented Oct 6, 2022

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
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
Contributor Author

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

losalex 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?

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

@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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: logging Issues related to the googleapis/nodejs-logging-bunyan API. size: s Pull request size is small.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Duplicate http request entries when running in Cloud Run
4 participants