-
Notifications
You must be signed in to change notification settings - Fork 7.6k
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
log: change log format to JSON payload for better log in Stackdriver #66
Conversation
…oogleCloudPlatform#47) change the log format in Python and Node.js services. Effected services are currencyservice, emailservice, paymentservice, and recommendationservice. Loadgenerator is left as is because of the diffculty to change the log format and log target in locust.
src/emailservice/logger.py
Outdated
import sys | ||
import json | ||
|
||
class JSONStreamHandler(logging.StreamHandler): |
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 wonder if there's an off-the-shelf library we can use for json logging.
It would be great to:
- keep the sample short by not writing this implementation
- not duplicate it between two python services
so I tend to think if you can find an external library, it can work better.
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.
Gotcha. python-json-logger should support it.
I'll replace the code to use this lib.
@ymotongpoo thanks a lot. left a comment. I wouldn't bother converting loadgenerator into structured logging, we don't consider it as a "service" to be monitored. |
|
||
# TODO(yoshifumi) this class is duplicated since other Python services are | ||
# not sharing the modules for logging. | ||
class CustomJsonFormatter(jsonlogger.JsonFormatter): |
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 it possible to avoid this file altogether? (since we're still duplicaing it.)
I want to think initializing a SD-compatible logging class shouldn't be 40 lines.
If there's any examples of logging to SD from 12-factor Python apps, I think we should try to reuse those patterns.
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.
(Sorry I squashed the commits for better commit logs)
I agree that it would be nice if we can use a neat logging library which enable us to create SD compatible JSON structured logs. I'm still searching the nicer library than python-json-logger but it seems that's the best option so far.
One thing I want to emphasize here is that in Python it's really common to use logging
standard library and create custom Handler or Formatter for desired log format, even if the format is flat text format, because in Python logging lib, the default format of log levels are in integers and level names are alias of those. In order to extract log level string, we need to have some custom class to extract levelname
data from record
object.
Also, the duplication happened due to the project structure. If we can share the module across all Python services, this problems never happens. Moreover, as you say, it's best for SD Logging to provide one-stop library for this use case.
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 see. I just started a thread internally to see maybe if there's an utility library we're missing. Truth to be told, this was much easier in Go. Initializing a JSON logger took 2 lines and 5 more lines to specify the custom fields. (We didn't even need to create a new method for it.) Let's wait and see what comes out of internal thread.
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.
Per offline thread, we can go ahead with the duplicated class implementation. If stop isolating services to their own directories somehow/someday, we could extract into a module for reuse in the future. 👍
python-json-logger is one of the most popular JSON formatter for Python logging and it requires a custom class to fit the log spec of Stackdriver Logging. As Python projects in this project do not share the logger module, the custom logger module needs to be duplicated in each micro services.
change the log format in Python and Node.js services.
Effected services are currencyservice, emailservice, paymentservice,
and recommendationservice. Loadgenerator is left as is because of
the diffculty to change the log format and log target in locust.
ref. #47