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: [bots] remove wait for logger #3711

Merged
merged 3 commits into from
Dec 17, 2021

Conversation

chrismaree
Copy link
Member

Motivation

The waitForLogger utility is extremely inconsistant in production and has proven to be dangerous to use. If this utility is called before all GCP logs have fully flushed out of the stream the process will hang until it times out, producing no logs. This makes it very difficult to figure out what's going wrong with the bots and to debug.

Unfortunately, there is no easy solution to this. Ideally, we want a utility that can halt code execution until all streams have been fully flushed (i.e all upstream messages sent). This is not posible with Winston. Note that their documentation claims this is posible but in practice it just does not work. See this PR for the best solution I've found (which inspired the original implementation of waitForLogger). Note that the code here does not work any more with GCP's @google-cloud/logging-winston package.

There are a number of people complaining about the same issue with @google-cloud/logging-winston. see issue 502 and issue 598 and there is no clear solution for now.

Details

Removes all usage of waitForLogger instances and replaces with a delay.

Testing

Check a box to describe how you tested these changes and list the steps for reviewers to test.

  • Ran end-to-end test, running the code as in production
  • New unit tests created
  • Existing tests adequate, no new tests required
  • All existing tests pass
  • Untested

Signed-off-by: chrismaree <[email protected]>
Signed-off-by: chrismaree <[email protected]>
Copy link
Member

@mrice32 mrice32 left a comment

Choose a reason for hiding this comment

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

Thanks for debugging this!

@chrismaree chrismaree merged commit a06c0f3 into master Dec 17, 2021
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.

2 participants