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

log: change log format to JSON payload for better log in Stackdriver #66

Merged
merged 2 commits into from Oct 5, 2018

Conversation

ymotongpoo
Copy link
Member

@ymotongpoo ymotongpoo commented Oct 2, 2018

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

…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.
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Oct 2, 2018
import sys
import json

class JSONStreamHandler(logging.StreamHandler):
Copy link
Contributor

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.

Copy link
Member Author

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.

@ahmetb
Copy link
Contributor

ahmetb commented Oct 2, 2018

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

@ymotongpoo ymotongpoo changed the title log: change log format to JSON payload for better log in Stackdriver (#47) log: change log format to JSON payload for better log in Stackdriver Oct 3, 2018

# TODO(yoshifumi) this class is duplicated since other Python services are
# not sharing the modules for logging.
class CustomJsonFormatter(jsonlogger.JsonFormatter):
Copy link
Contributor

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.

Copy link
Member Author

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.

Copy link
Contributor

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.

Copy link
Contributor

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.
@ahmetb ahmetb merged commit 7f40378 into GoogleCloudPlatform:master Oct 5, 2018
tjohander-splunk pushed a commit to tjohander-splunk/microservices-demo-google that referenced this pull request Dec 17, 2022
D-Mwanth pushed a commit to D-Mwanth/microservices-demo that referenced this pull request Mar 6, 2024
…oogleCloudPlatform#66)

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. GoogleCloudPlatform#47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants