-
Notifications
You must be signed in to change notification settings - Fork 234
feat: ad-hoc, targeted debug logging #2062
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
base: main
Are you sure you want to change the base?
Conversation
Co-authored-by: Alex Hong <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same general comment about private elements
src/publisher/message-queues.ts
Outdated
} | ||
|
||
/** | ||
* Cancels any pending publishes and calls _publish immediately. | ||
* | ||
* Does _not_ attempt to further drain after one batch is sent. | ||
*/ | ||
async publish(): Promise<void> { | ||
await this._publishInternal(false); | ||
async publish(reason: string): Promise<void> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since you're adding a param, wouldn't you need to add a default value to make it backwards-compatible or make it optional?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These two are internal/private methods, so they shouldn't need a default. I'm trying to make sure that everything that calls them gets updated for the logging, but at this point I guess it would be okay to add a default? Users definitely shouldn't be calling these directly, so I am also fine with just leaving it as-is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, re: private methods, this library is pretty old and wasn't built to use them, so it would be pretty painful to add it now, imo. I'll definitely think about that going forward, but e.g. a lot of the unit tests assume that innards are accessible for mocking. I'd like to find a better way to do the unit tests too, but there are over 900 of them now 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I went ahead and made those optional parameter changes.
This feature will add the ability to selectively enable "printf debugging" output for diagnostic introspection. This uses the new standard Google ad-hoc debug logging infrastructure introduced a few months back.
Samples:
All hand-written logic on top of the auto-generated RPCs.
All logging related to Pub/Sub (including the RPCs).
All logging, including things like auth.
A fuller guide will be forthcoming.