-
Notifications
You must be signed in to change notification settings - Fork 87
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 adhoc debug logger package #1669
base: main
Are you sure you want to change the base?
Conversation
Here is the summary of possible violations 😱 There are 2 possible violations for not having product prefix.
The end of the violation section. All the stuff below is FYI purposes only. Here is the summary of changes. You are about to add 2 region tags.
This comment is generated by snippet-bot.
|
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 took a quick look and only had really teeny nits about package name agreement
@@ -1,4 +1,5 @@ | |||
{ | |||
"gax": "4.5.0", | |||
"tools": "0.4.6" | |||
"tools": "0.4.6", | |||
"logging-utils": "0.0.0" |
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.
name and version disagree with what's in package.json (google-logging-utils
and 0.0.2
) - should these match?
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.
Hopefully the 0.0.2 is published experimentally! Also - is this name already taken? https://www.npmjs.com/package/logging-utils
"fix": "gts fix", | ||
"prepare": "npm run compile", | ||
"precompile": "gts clean", | ||
"samples-test": "echo no samples 🙀", |
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.
There is a sample, should there either be a test or should this actually say no samples tests?
"directory": "adhoc-logging/samples" | ||
}, | ||
"engines": { | ||
"node": ">=14" | ||
}, | ||
"dependencies": { | ||
"@google-cloud/adhoc-logging": "0.0.1" |
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.
should these be updated to say logging-utils
and not adhoc-logging
?
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.
Plus one, I think this should be logging-utils!
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.
Generally, LGTM. However, it would be cool to see it in action in a system test, something like gax's showcase-client. But since we haven't added code into our generated libraries I could see this being more of a to-do.
"directory": "adhoc-logging/samples" | ||
}, | ||
"engines": { | ||
"node": ">=14" | ||
}, | ||
"dependencies": { | ||
"@google-cloud/adhoc-logging": "0.0.1" |
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.
Plus one, I think this should be logging-utils!
* variables) to determine what logging they want to see at runtime. This | ||
* isn't necessarily fed into the console, but is meant to be under the | ||
* control of the user. The kind of logging that will be produced by this | ||
* is more like "call retry happened", not "event you'd want to record |
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.
* is more like "call retry happened", not "event you'd want to record | |
* is more like "call retry happened", not "events you'd want to record |
assert.deepStrictEqual(sink.logs, []); | ||
}); | ||
|
||
it('obeys "all"', () => { |
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.
nit: more descriptive test name? What should 'all' be returning?
Browser tests are failing because I moved them to Node 18. You can wait for this PR to be merged first, or just copy over the .kokoro/**/*browser.cfg/setup/run files from that PR. Or I could admin merge it, since it really doesn't change anything in gax - depends on the speed with which you need it! |
This adds a new pseudo-mono-repo package for adhoc debug logging. It's meant to be pulled in across any library and/or GAPIC to provide the ability to do the equivalent of "printf debugging", but with filtering and a disable flag.
Working group spec doc available upon request :)