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 #1671

Open
wants to merge 23 commits into
base: main
Choose a base branch
from

Conversation

feywind
Copy link
Contributor

@feywind feywind commented Dec 19, 2024

This adds request/response logging for all gapic libraries, in both cjs and esm.

This is a duplicate of another PR to comply with some process changes.

@feywind feywind requested review from a team as code owners December 19, 2024 21:54
@feywind
Copy link
Contributor Author

feywind commented Dec 19, 2024

FYI, there are a couple of meaty comments on the older PR that should be addressed here. Also we don't plan to actually regen based on this until after EOY.

@feywind
Copy link
Contributor Author

feywind commented Feb 12, 2025

Found during this work and put aside for now: #1684

@feywind
Copy link
Contributor Author

feywind commented Feb 12, 2025

Moving this reply to @sofisl over here so I can close the other PR:

  • 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.

Sounds good!

  • 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).

I don't quite understand about the double-printing, do you mean that we'd be printing stuff from the request in the response? I think, in general, there shouldn't be duplication.

  • Unfortunately we'll have to make these changes in the ESM side of things as well!

Doh. Yes. (update: these should also be done now)

  • 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.

I will take a look, but the intention of wrappedCallback is that it's supposed to have a nullness equivalent to what the user passed in for the callback. So if it worked before, it should work now. (Or not, in which case, fixes...)

(update: These particular issues should be fixed now, but it looks like some other stuff remains.)

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!

Okay, that sounds fine to me. You'd know more about the structure of these particular packages than me. :)

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.

1 participant