add support to parse JSON logs #228

niamster · 2023-02-11T20:03:38Z

Addressed #227

This PR adds:

add --input-format option that allows specifying message input format
add zap-json input format support

…, and add zap-json input format support

superbrothers · 2023-02-12T08:22:02Z

cmd/cmd.go

+		if color.NoColor {
+			messageTpl = `{{ with $msg := .Message | parseJSON }}[{{ $msg.ts }}] {{ $msg.level }} ({{ $msg.caller }}) {{ $msg.msg }}{{ end }}`
+		} else {
+			messageTpl = `{{ with $msg := .Message | parseJSON }}[{{ color $.TimestampColor $msg.ts }}] {{ levelColor $msg.level }} ({{ color $.CallerColor $msg.caller }}) {{ $msg.msg }}{{ end }}`


I would like to create a generic color function and use it as follows. This will eliminate the need to change tail.go.

diff --git a/cmd/cmd.go b/cmd/cmd.go index 5dc6784..4395441 100644 --- a/cmd/cmd.go +++ b/cmd/cmd.go @@ -242,7 +242,7 @@ func (o *options) sternConfig() (*stern.Config, error) { if color.NoColor { messageTpl = `{{ with $msg := .Message | parseJSON }}[{{ $msg.ts }}] {{ $msg.level }} ({{ $msg.caller }}) {{ $msg.msg }}{{ end }}` } else { - messageTpl = `{{ with $msg := .Message | parseJSON }}[{{ color $.TimestampColor $msg.ts }}] {{ levelColor $msg.level }} ({{ color $.CallerColor $msg.caller }}) {{ $msg.msg }}{{ end }}` + messageTpl = `{{ with $msg := .Message | parseJSON }}[{{ colorGreen $msg.ts }}] {{ levelColor $msg.level }} ({{ colorCyan $msg.caller }}) {{ $msg.msg }}{{ end }}` } case "default": default: @@ -312,6 +312,14 @@ func (o *options) sternConfig() (*stern.Config, error) { "color": func(color color.Color, text string) string { return color.SprintFunc()(text) }, + "colorBlack": color.BlackString, + "colorRed": color.RedString, + "colorGreen": color.GreenString, + "colorYellow": color.YellowString, + "colorBlue": color.BlueString, + "colorMagenta": color.MagentaString, + "colorCyan": color.CyanString, + "colorWhite": color.WhiteString, "levelColor": func(level string) string { var levelColor *color.Color switch strings.ToLower(level) {

Thank you for the suggestion! I was not happy about adding specific color fields into the tail.go file, and I was thinking more about function like colorFromString, or even change default color function to test agains specific constants, but I was not sure whether it worth the trouble.
I like your idea, will apply your suggestion 👍

superbrothers · 2023-02-12T08:24:19Z

README.md

@@ -84,6 +84,7 @@ Supported Kubernetes resources are `pod`, `replicationcontroller`, `service`, `d
 `--field-selector`          |           | Selector (field query) to filter on. If present, default to ".*" for the pod-query.
 `--include`, `-i`           | `[]`      | Log lines to include. (regular expression)
 `--init-containers`         | `true`    | Include or exclude init containers.
+ `--input-format`, `-I`      | `default` | Specify input format. Currently support: [default, zap-json]


Since the name of the option that specifies the output format is output, it seems that the name of the option that specifies the input format should be input, but I am torn.

I'm a little bit hesitant about --input since it's too broad. If one day someone would like to add --input option for whatever reason, that person would have hard time finding an appropriate name.

I feel that the flag name --input-format is a bit confusing because its purpose is to change the output format. We can consider it a formatter for json-zap, so how about --formatter?

stern --formatter zap-json <QUERY>

Yeah, I was afraid that --input-format might sound misleading, one of the option I had in mind is --reformat 😆 .

--formatter sounds good for me, @superbrothers what do you think?

I've changed --input-format to --formatter.

…late constants, update template accordingly

tksm · 2023-02-13T00:05:21Z

Thank you for the PR!

I understand the use case, but I would prefer not to support application or library-specific formats to keep Stern simple. There are countless log libraries and log formats, so it will soon be unmanageable in this approach.

I believe doing the log formatting and the syntax highlighting in another program is better. It is more flexible and works better with other programs.

stern <QUERY> | read-zap-json

superbrothers · 2023-02-13T05:32:51Z

There are countless log libraries and log formats, so it will soon be unmanageable in this approach.

Yes, you may be right. We need to continue to manage once we support. However, since there is no change to the default, it seems to me that if it breaks, the person who needs it can just fix it.

tksm · 2023-02-13T05:42:51Z

However, since there is no change to the default, it seems to me that if it breaks, the person who needs it can just fix it.

That makes sense. In that case, I have no objection to adding this feature.

niamster · 2023-02-13T21:55:54Z

Thank you for the PR!

I understand the use case, but I would prefer not to support application or library-specific formats to keep Stern simple. There are countless log libraries and log formats, so it will soon be unmanageable in this approach.

I believe doing the log formatting and the syntax highlighting in another program is better. It is more flexible and works better with other programs.
stern <QUERY> | read-zap-json

Thank you for taking time to review 🙇

I perfectly understand your reasoning, and TBH I was considering using something like https://github.com/garabik/grc. At the same time it looks an overkill, and stern already's doing 99% of useful work.

I was thinking that at some point these templates could be moved to configuration files, and even have a dedicated "community" repo if their number starts to grow, but it's too early to make such decision.

tksm · 2023-02-13T23:28:28Z

I was thinking that at some point these templates could be moved to configuration files, and even have a dedicated "community" repo if their number starts to grow, but it's too early to make such decision.

I agree it's too early to move to configuration files, so I favor implementing it as a flag. 👍

niamster · 2023-02-16T10:04:09Z

@tksm @superbrothers Thank you for taking your time for the review! Is there anything else I could do to get this PR approved and merged?

tksm

LGTM 🎉

tksm · 2023-02-16T12:47:57Z

@superbrothers Please check the change of the flag name to --formatter to be sure. I'll leave it to you to merge.

niamster · 2023-02-17T08:33:33Z

Thank you folks 🙇

tksm · 2023-02-21T00:26:02Z

@niamster I tried this feature and found the following problems. Could you check them?

I assume the format of zap-json is as follows. https://go.dev/play/p/6UfoGdeqJ9f

{"level":"info","ts":1257894000,"caller":"sandbox2210433069/prog.go:10","msg":"hello","key1":"val1"}
{"level":"info","ts":1257894000,"caller":"sandbox2210433069/prog.go:13","msg":"world","key1":"val1"}

When we specify --color always, it fails with the following errors. It might be necessary to convert ts to a string (or a human-readable date).

$ dist/stern --color always --formatter zap-json pods/zap-log --no-follow --only-log-lines
expanding template failed: template: log:1:128: executing "log" at <$msg.ts>: wrong type for value; expected string; got float64
expanding template failed: template: log:1:128: executing "log" at <$msg.ts>: wrong type for value; expected string; got float64

When we specify --color never, it works, but fields other than msg are missing. Is this what you intended?

$ dist/stern --color never --formatter zap-json pods/zap-log --no-follow --only-log-lines
zap-log c [1.257894e+09] info (sandbox2210433069/prog.go:10) hello
zap-log c [1.257894e+09] info (sandbox2210433069/prog.go:13) world

I tested with the following manifest.

apiVersion: v1
kind: Pod
metadata:
  name: zap-log
spec:
  restartPolicy: Never
  containers:
  - command:
    - sh
    - -c
    - |
      cat <<EOS
      {"level":"info","ts":1257894000,"caller":"sandbox2210433069/prog.go:10","msg":"hello","key1":"val1"}
      {"level":"info","ts":1257894000,"caller":"sandbox2210433069/prog.go:13","msg":"world","key1":"val1"}
      EOS
    image: busybox:1.36.0
    name: c

niamster · 2023-02-21T08:49:17Z

@niamster I tried this feature and found the following problems. Could you check them?

I assume the format of zap-json is as follows. https://go.dev/play/p/6UfoGdeqJ9f

{"level":"info","ts":1257894000,"caller":"sandbox2210433069/prog.go:10","msg":"hello","key1":"val1"}
{"level":"info","ts":1257894000,"caller":"sandbox2210433069/prog.go:13","msg":"world","key1":"val1"}

When we specify --color always, it fails with the following errors. It might be necessary to convert ts to a string (or a human-readable date).

$ dist/stern --color always --formatter zap-json pods/zap-log --no-follow --only-log-lines
expanding template failed: template: log:1:128: executing "log" at <$msg.ts>: wrong type for value; expected string; got float64
expanding template failed: template: log:1:128: executing "log" at <$msg.ts>: wrong type for value; expected string; got float64

When we specify --color never, it works, but fields other than msg are missing. Is this what you intended?

$ dist/stern --color never --formatter zap-json pods/zap-log --no-follow --only-log-lines
zap-log c [1.257894e+09] info (sandbox2210433069/prog.go:10) hello
zap-log c [1.257894e+09] info (sandbox2210433069/prog.go:13) world

I tested with the following manifest.

apiVersion: v1
kind: Pod
metadata:
  name: zap-log
spec:
  restartPolicy: Never
  containers:
  - command:
    - sh
    - -c
    - |
      cat <<EOS
      {"level":"info","ts":1257894000,"caller":"sandbox2210433069/prog.go:10","msg":"hello","key1":"val1"}
      {"level":"info","ts":1257894000,"caller":"sandbox2210433069/prog.go:13","msg":"world","key1":"val1"}
      EOS
    image: busybox:1.36.0
    name: c

Hi Takashi, I have a different zap output format:

{"level":"INFO","ts":"2023-02-20T17:56:27.238272+00:00", ...

You can see that the ts field is string and not an integer. I dug a little bit and it seems that in my environment a custom TS encoding is used for historical reasons.
Do you want me to revert this PR and propose some different approach or some fix-forward PR is better?

Thank you for your understanding!

tksm · 2023-02-21T09:02:28Z

Thank you for confirming!

Do you want me to revert this PR and propose some different approach or some fix-forward PR is better?

If you plan to fix it soon, either is fine for me. I prefer to revert if it takes some time because it might be included in the next release.

This reverts commit 72a5854.

niamster · 2023-02-21T09:07:49Z

Thank you for confirming!

Do you want me to revert this PR and propose some different approach or some fix-forward PR is better?

If you plan to fix it soon, either is fine for me. I prefer to revert if it takes some time because it might be included in the next release.

Let's revert then to avoid any problems #232

I will propose a new PR later when I have more time to investigate possible solutions.
Thank you for your understanding.

This reverts commit 72a5854.

add --input-format option that allows specifying message input format…

ea88f92

…, and add zap-json input format support

niamster requested review from superbrothers, floryut, rkmathi and tksm as code owners February 11, 2023 20:03

niamster mentioned this pull request Feb 11, 2023

Add support to parse custom JSON logs #227

Open

update readme

58a4ea2

superbrothers reviewed Feb 12, 2023

View reviewed changes

add colorXXX functions and remove TimestampColor and CallerColor temp…

20b7ab1

…late constants, update template accordingly

niamster requested review from superbrothers and removed request for rkmathi, tksm and floryut February 12, 2023 17:01

niamster added 2 commits February 15, 2023 10:44

change --input-format to --formatter

9bbe168

Merge branch 'master' into dm/zap-dec

79286da

tksm approved these changes Feb 16, 2023

View reviewed changes

superbrothers approved these changes Feb 17, 2023

View reviewed changes

superbrothers merged commit 72a5854 into stern:master Feb 17, 2023

niamster added a commit to niamster/stern that referenced this pull request Feb 21, 2023

Revert "add support to parse JSON logs (stern#228)"

a7b8919

This reverts commit 72a5854.

niamster mentioned this pull request Feb 21, 2023

Revert "add support to parse JSON logs" #232

Merged

tksm pushed a commit that referenced this pull request Feb 21, 2023

Revert "add support to parse JSON logs (#228)" (#232)

202f7e8

This reverts commit 72a5854.

niamster mentioned this pull request Feb 26, 2023

allow flexible log parsing and formatting #239

Merged

add support to parse JSON logs #228

add support to parse JSON logs #228

Uh oh!

Conversation

niamster commented Feb 11, 2023

Uh oh!

superbrothers Feb 12, 2023

Choose a reason for hiding this comment

Uh oh!

niamster Feb 12, 2023

Choose a reason for hiding this comment

Uh oh!

superbrothers Feb 12, 2023

Choose a reason for hiding this comment

Uh oh!

niamster Feb 12, 2023

Choose a reason for hiding this comment

Uh oh!

tksm Feb 13, 2023

Choose a reason for hiding this comment

Uh oh!

niamster Feb 13, 2023

Choose a reason for hiding this comment

Uh oh!

niamster Feb 15, 2023

Choose a reason for hiding this comment

Uh oh!

tksm commented Feb 13, 2023

Uh oh!

superbrothers commented Feb 13, 2023

Uh oh!

tksm commented Feb 13, 2023

Uh oh!

niamster commented Feb 13, 2023

Uh oh!

tksm commented Feb 13, 2023

Uh oh!

niamster commented Feb 16, 2023

Uh oh!

tksm left a comment

Choose a reason for hiding this comment

Uh oh!

tksm commented Feb 16, 2023

Uh oh!

niamster commented Feb 17, 2023

Uh oh!

tksm commented Feb 21, 2023

Uh oh!

niamster commented Feb 21, 2023

Uh oh!

tksm commented Feb 21, 2023

Uh oh!

niamster commented Feb 21, 2023

Uh oh!

Uh oh!