fix: skip writing RequestLogEntry for Cloud Run with flag enabled #821
Conversation
|
Warning: This pull request is touching the following templated files:
|
product-auto-label
bot
added
size: s
cindy-peng
changed the title
cindy-peng
changed the title
| @@ -35,15 +35,18 @@ type Middleware = ReturnType<typeof commonMiddleware.express.makeMiddleware>; | |||
|
|
|||
| export async function makeMiddleware( | |||
| logger: winston.Logger, | |||
| transport: LoggingWinston | |||
| transport: LoggingWinston, | |||
| skipParentEntryForCloudRun?: boolean | |||
There was a problem hiding this comment.
Is this new parameter fully optional? (That is, could this break existing code?)
There was a problem hiding this comment.
Yes Daniel, this new parameter is fully optional.
| @@ -303,6 +303,8 @@ decrease logging record loss upon execution termination - since all logs are wri | |||
| would be picked up by the Cloud Logging Agent running in Google Cloud managed environment. | |||
| Note that there is also a `useMessageField` option which controls if "message" field is used to store | |||
| structured, non-text data inside `jsonPayload` field when `redirectToStdout` is set. By default `useMessageField` is always `true`. | |||
| Set the `skipParentEntryForCloudRun` option to skip creating an entry for the request itself as Cloud Run already automatically creates | |||
| such log entries. This might become the default behaviour in a next major version. | |||
There was a problem hiding this comment.
Personally I don't really like this as a solution, but since this was already implemented in nodejs-logging-bunyan, it does make sense to duplicate the fix as-is here.
It seems like this fix was meant to be temporary until the next major revision, when we can do a breaking change. Do you know if a bug is opened to track that? (I usually like to keep a milestone open in the issues list with changes that are waiting for the next breakinc change)
There was a problem hiding this comment.
Yes I agree, I wish I could find a more cleaner solution to this.
Regarding the tracking bug for the breaking change, I didn't find any in either this repo or in nodejs-logging-bunyan. Do you think we should open one in both repos to track it for the major release?
There was a problem hiding this comment.
Yeah, I like to use issue milestones to track major versions - and you can even have a branch for the next major version going when you're ready to start building changes for it.
But it's up to you how you want to organize things, just a suggestion!
There was a problem hiding this comment.
Yeah, I like to use issue milestones to track major versions - and you can even have a branch for the next major version going when you're ready to start building changes for it.
But it's up to you how you want to organize things, just a suggestion!
Makes sense! Thanks Daniel! For now I created this issue here to track the default behavior change for the major release: #824
Will explore how I can use milestones to track issues at different phases.
daniel-sanche
added
the
owlbot:run
gcf-owl-bot
bot
removed
the
owlbot:run
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 #795 🦕
This is a fix similar to the fix in nodejs-logging-bunyan to skip writting parent request log entries for cloud run.
To avoid breaking changes on the existing applications on Cloud Run, we introduce a optional parameter "skipParentEntryForCloudRun" to control the behavior. Right now the default behavior is still to create an request log entry unless "skipParentEntryForCloudRun" is set to be true specifically. We can change the default behavior in the next major release.