-
Notifications
You must be signed in to change notification settings - Fork 39
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 support for JSON logging to stderr #1027
Conversation
9657e1b
to
092f5df
Compare
c3a364d
to
c9f133c
Compare
🤖 I detect that the PR title and the commit message differ and there's only one commit. To use the PR title for the commit history, you can use Github's automerge feature with squashing, or use -- conventional-commit-lint 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.
@bpcreech , thanks for opening this PR! I have a conceptual feedback on this change - basically when you configure stdout
/stderr
to be used, it will redirect all logging traffic to stdout
/stderr
respectively. While developer always can create 2 loggers to be able to use both, stdout
and stderr
streams, it would be best to make this feature dependent on Severity. This way we can define more flexible logic to send log record to dedicated output streams based if the record has error-related severity or not (e.g. DEFAULT/DEBUG/INFO/NOTICE severities will go to stdout
and the rest to stderr
).
Thats said, I believe that Severity
based output could be introduced as separate feature in addition to ability to redirect everything to stdout
or stderr
- please let me know WDYT about this and we can take it from there.
google-cloud-logging/src/main/java/com/google/cloud/logging/LoggingHandler.java
Show resolved
Hide resolved
67d5996
to
42d0792
Compare
Hey @losalex and @bpcreech, I think this change made it so the default python logging package no longer logs to gcloud without using a structured log to STOUT |
Hmm, this change was to the Java logging library, which is not used by any Python SDK I'm aware of. |
I was thinking it may consume logs that are formatted via the python SDK, nothing directly related if that makes sense |
Can you describe your software stack? What environment are you running on, and what SDKs are you using? |
We run python scripts using kubernetes in gcloud,
This is pretty confusing to me though, google stackdriver doesn't have our logs but
|
if (autoPopulateMetadata) { | ||
logEntries = | ||
logging.populateMetadata( | ||
logEntries, getMonitoredResource(), "com.google.cloud.logging", "java"); | ||
} | ||
if (redirectToStdout) { | ||
logEntries.forEach(log -> System.out.println(log.toStructuredJsonString())); | ||
} else { |
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.
it stands out to me that this is an if else and is replaced with a switch without a default case, but 🤷
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.
@nickkieffer , are you using redirectToStdout
feature?
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.
no, not directly anyways
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.
@nickkieffer , I must admit that I have some confusion while following the issue you raised, but let me try answering best based on my understanding:
- Correct me if I wrong, you use a Python logger - based on sample code you shared. This change to print to
STDERR
was performed only in Java library and I believe we do not have such feature in Python. If problem indeed related to Python SDK, can you please raise the issue in Python repo? Feel free to provide all details you mentioned here and also the details about environment you run in - adding @daniel-sanche for visibility. - This change supposed to use
CLOUD_LOGGING
by default - theSTDOUT
andSTDERR
logging needs to be explicitly enabled in code or configuration so the change should be fully backward compatible to clients which does not use the feature. If you believe this is not a case, please let us know - me and @bpcreech can take a look.
Please let me know if I understood you correctly and if I can assist you any further.
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.
You can disregard me on this, I think my ops team just so happened to turn off of logging agents around the same time as your release happened. Sorry about the distraction!
Fixes #1026