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 debug logging support #1898

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

Conversation

feywind
Copy link
Contributor

@feywind feywind commented Nov 22, 2024

Adds debug logging support for auth. This needs a bit more work/testing still.

It's also waiting on this to finish:
googleapis/gax-nodejs#1669

@product-auto-label product-auto-label bot added the size: m Pull request size is medium. label Nov 22, 2024
@feywind feywind marked this pull request as ready for review December 10, 2024 18:42
@feywind feywind requested review from a team as code owners December 10, 2024 18:42
@feywind
Copy link
Contributor Author

feywind commented Dec 10, 2024

I need to update this for when the debug logger piece is officially reviewed and released, and there's a system test thing to look at, but the code itself is largely ready, I think.

Comment on lines 23 to 24
import {log as makeLog} from 'google-logging-utils';
const log = makeLog('auth');
Copy link

Choose a reason for hiding this comment

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

I think it may be cleaner and flexible to add this to the base AuthClient class and have it as a member or static method.

This will allow:

  • Customers to change/modify the log functionality (if they want)
  • Dedupe the import statements across the files

Additionally, in the next major we're planning to unify the Transporter instance into AuthClient via this PR:

It would be an even smaller migration after that has landed as we could extend the GaxiosOptions interface with a Symbol (which would note the type of log) and use log.info on every request (as all requests would use that _request method).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the quick review. I'll make that change tomorrow.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A lot of the logging can be done through AuthClient now, good call. There are a few objects that don't seem to derive ultimately from AuthClient, so I moved makeLog() into the class itself to make it cleaner. (makeLog caches the loggers by name, so there's little harm in calling it multiple times.)

@feywind feywind added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Dec 20, 2024
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Dec 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size: m Pull request size is medium.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants