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

feat: add request/response debug logging to gapics #1670

Draft
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

feywind
Copy link
Contributor

@feywind feywind commented Dec 3, 2024

Work in progress, creating a PR for visibility.

Copy link
Contributor

@sofisl sofisl left a comment

Choose a reason for hiding this comment

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

  • I think now I'd want to see a few test cases covered, specifically some integration tests like, should render logging for a ${streaming, LRO, etc.} method like so, and I think it would be good to confirm formatting and inclusion of the key pieces of information.
  • I'm a bit worried that the logging will be noisy, i.e., this._log.info('{{ method.name.toCamelCase() }} response %j', response);. Would we potentially be double-printing the response with logging? (Not quite sure how this is supposed to work with the design).
  • Unfortunately we'll have to make these changes in the ESM side of things as well!
  • What happens if wrappedCallback is undefined? it would be good to see the call still go through. Edit: I think this is what the library tests are currently complaining about! Likewise it would be nice to have a test where logging is disabled and we still see the call go through.

For the integration tests, I think it would be good to have a set of tests in the gax layer that regenerates showcase and confirms the test cases described above. I'm thinking debug-logging can pull in the showcase directory in gax and run some tests - but that's just my rough idea, I think there's many ways we could add these integration tests. LMK if you want help in setting this up, I think a good way of doing this is to regenerate showcase using this branch of the generator, then add some tests, then once everything looks ok we can officially merge this branch and release a new version!

@@ -97,6 +99,7 @@ export class API {
// users specify the actual package name, if not, set it to product name.
this.publishName =
options.publishName || this.naming.productName.toKebabCase();
this.loggingName = this.publishName.replace(/^@google-cloud\//, '');
Copy link
Contributor

Choose a reason for hiding this comment

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

this can vary greatly (i.e., googleshopping, google-ads, google-cloud, googleapis, etc.), I think probably best just to use a capturing group after the first backslash

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