Skip to content

Conversation

hogklint
Copy link
Contributor

@hogklint hogklint commented Nov 9, 2024

This fixes #320

This PR extends the functionality of the template function levelColor to also support numeric values. Try it out with

for l in 10 20 30 40 50 60 70 \"debug\" \"info\" \"warn\" \"error\" \"fatal\"; do echo '{"level": '$l', "msg": "A message"}'; done | stern --stdin --template='{{with $d := .Message | parseJSON}}[{{levelColor $d.level}}] {{$d.msg}}{{end}}{{"\n"}}'

In case it cannot parse the value as a string or number it currently falls back to returning an empty string without an error. I think this is quite nice as it allows for things like this {{or (colorLevel $d.level) "N/A"}} if the level is missing completely. It is however a behavior change.

Furthermore, it can be debated whether the levels I've chosen make the most sense. They are set according to what bunyan and pino uses whereas log4j use levels in the 100s. A flag to switch between the two styles perhaps?

@tksm
Copy link
Contributor

tksm commented Nov 10, 2024

Hi, thanks for the PR!

I think it would be best to avoid introducing a behavior change for existing users. How about creating a new function, such as numLevelColor() or bunyanLevelColor(), instead?

@hogklint
Copy link
Contributor Author

hogklint commented Nov 10, 2024

Thanks for your reply.

How about creating a new function, such as numLevelColor() or bunyanLevelColor(), instead?

If you tail two containers simultaneously, one which uses string levels, and one which uses numeric levels, how could one solve that if there are different color functions with the current behavior of levelColor? I haven't been able to catch the error thrown when the type doesn't match so that it could fall back to a numeric level parsing.

If adding a bunyanLevelColor and if levelColor could be changed to be more lenient this should at least be possible, right? {{or (levelColor $d.level) (bunyanLevelColor $d.level)}}. If something similar can be achieved with the current behavior of levelColor I would be happy with that, otherwise, I want to suggest this behavior change.

Edit: And by "this behavior change" I don't mean what's currently in the PR. I like splitting the color functions but to enable versatility it would be good if levelColor does not throw an error.

@tksm
Copy link
Contributor

tksm commented Nov 11, 2024

If adding a bunyanLevelColor and if levelColor could be changed to be more lenient this should at least be possible, right? {{or (levelColor $d.level) (bunyanLevelColor $d.level)}}. If something similar can be achieved with the current behavior of levelColor I would be happy with that, otherwise, I want to suggest this behavior change.

How about calling bunyanLevelColor() before levelColor(), instead of changing the current behavior of levelColor()? This approach is more flexible because we could add another level color function such as log4jLevelColor() in the future.

for l in 10 20 30 40 50 60 70 \"debug\" \"info\" \"warn\" \"error\" \"fatal\";
  do echo '{"level": '$l', "msg": "A message"}'
done | \
dist/stern --stdin \
  --template='{{with $d := .Message | parseJSON}}[{{or (bunyanLevelColor $d.level) (levelColor $d.level)}}]  {{$d.msg}}{{end}}{{"\n"}}'

image

		"bunyanLevelColor": func(level any) string {
			var lv int64
			var err error

			switch value := level.(type) {
			// tryParseJSON yields json.Number
			case json.Number:
				lv, err = value.Int64()
				if err != nil {
					return ""
				}
			// parseJSON yields float64
			case float64:
				lv = int64(value)
			default:
				return ""
			}

			switch {
			case lv < 30:
				return color.New(color.FgMagenta).SprintFunc()(lv)
			case lv < 40:
				return color.New(color.FgBlue).SprintFunc()(lv)
			case lv < 50:
				return color.New(color.FgYellow).SprintFunc()(lv)
			case lv < 60:
				return color.New(color.FgRed).SprintFunc()(lv)
			case lv < 100:
				return color.New(color.FgCyan).SprintFunc()(lv)
			default:
				return strconv.FormatInt(lv, 10)
			}
		},

@hogklint
Copy link
Contributor Author

Yeah, sure we can go with that. I just want to point out a couple of things.

  • I'm not sure I understand the reason for keeping the current levelColor behavior. The error causes the log line to be discarded which seems undesireable to me.
  • Having bunyanLevelColor fall back to an empty string creates a behavior difference between it and levelColor although as a user I would expect them to function similarly.
    • E.g. this {{or (levelColor $d.level) (bunyanLevelColor $d.level)}} and this {{or (bunyanLevelColor $d.level) (levelColor $d.level)}} will behave differently even though I would expect the outcome to be the same. Feels like arcane knowledge (which I can try to add to the README).

I'll make the changes, but do let me know if you change your mind. 👍

@tksm
Copy link
Contributor

tksm commented Nov 13, 2024

I'm not sure I understand the reason for keeping the current levelColor behavior. The error causes the log line to be discarded which seems undesireable to me.

I have no objection to changing the levelColor argument type to any (interface{}), as long as backward compatibility is maintained. I missed your edited comment, so I misunderstood that you wanted to add an error return value, which would break backward compatibility.

@hogklint
Copy link
Contributor Author

Ok, cool. 👍 Even adding the error return value doesn't affect the user. The template engine swallows it and prints it. But I'll stick with the single string return.

levelColor discarded log lines if it could not coerce the given value to
a string. By accepting anything the user has the option to handle such
cases in the template instead.
@hogklint
Copy link
Contributor Author

All done, let me know if you want to change anything.

Copy link
Contributor

@tksm tksm left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

@tksm tksm merged commit db69276 into stern:master Nov 14, 2024
2 checks passed
@hogklint hogklint deleted the level-number branch November 14, 2024 16:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Colorize numeric log levels
2 participants