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

Change log format to JSON and log destination to stdout #48

Merged
merged 5 commits into from
Sep 25, 2018

Conversation

ymotongpoo
Copy link
Contributor

@ymotongpoo ymotongpoo commented Sep 24, 2018

This is the PR for #47. This PR effects all the services written in Go.

The summary of this PR is:

  1. Change log format from text payload to JSON payload.
  2. Change log destination from stderr to stdout.
  3. Change target container repository to mine so that I can test changes from 1 and 2.
  4. Change scenario of locust test.

For 3 and 4, I'll revert them to the original once the objective is approved.

change the log format in Go written service from text payload to
JSON payload using 3rd party logging library (logrus).

https://cloud.google.com/logging/docs/structured-logging
https://github.com/sirupsen/logrus/blob/33a1e118e113c7d1dd24a80f80670864cd598519/json_formatter.go#L40-L49

Effected services are frontend, productcatalogservice, checkoutservice,
and shippinservice.

Also change target container registry and locust scenario for testing.
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Sep 24, 2018
@ahmetb
Copy link
Contributor

ahmetb commented Sep 24, 2018

Change log destination from stderr to stdout.

I'm not sure if stderr tells anything about the log to Stackdriver. In my https://github.com/ahmetb/coffeelog project I've seen that output stream doesn't matter as long as you get the Stackdriver json fields right.

As for (3) and (4), not sure why you felt like you did "git add" these files. 😄 You can test those without adding them to your commit.

@@ -50,6 +50,6 @@ func quoteByCountFloat(count int) float64 {
return 0
}
count64 := float64(count)
var p float64 = 1 + (count64 * 0.2)
var p = 1 + (count64 * 0.2)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure what this line changes. isn't it already float64?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

var p doesn't require float64 type definition as it's inferred from count64. Moreover now I realized it doesn't require var keyword as well.

p := 1 + (count64 * 0.2) just works.

Copy link
Contributor

@ahmetb ahmetb left a comment

Choose a reason for hiding this comment

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

This PR is good (minus changes to yamls)! Especially liked that you got the Stackdriver logging fields right. 👌 Thanks a lot for the work.

Current questions I have:

  1. why only Go services? do we need other languages to move to structured logging too?
  2. we can probably set out to stderr and get the same result?

@ymotongpoo
Copy link
Contributor Author

ymotongpoo commented Sep 25, 2018

Thanks @ahmetb for taking a look.

As for (3) and (4), not sure why you felt like you did "git add" these files. smile You can test those without adding them to your commit.

You're right. They should have omitted 😅

Answering the following questions

  1. why only Go services? do we need other languages to move to structured logging too?

This is just a first step. Once this PR merged, I'll work on other services written in other languages.

  1. we can probably set out to stderr and get the same result?

https://cloud.google.com/kubernetes-engine/docs/how-to/logging
At least when I tested it, Stackdriver Logging treated logs to stderr as Error even if they are structured logs and their severity is properly set. It sounds the spec of the Stackdriver Logging.
As of now, I put logs as stdout as the tentative solution.

@ahmetb ahmetb merged commit 6460427 into GoogleCloudPlatform:master Sep 25, 2018
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.

3 participants