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

Adding JSON formatted log feature #120

Merged
merged 4 commits into from
Jan 30, 2019

Conversation

syedimam0012
Copy link

@syedimam0012 syedimam0012 commented Jan 25, 2019

What this is about?

This feature allows having JSON formatted (i.e. structured) logs. This means targeted querying and filtering can be performed in e.g. Grafana, Kibana, and similar dashboard tools.

Tested?

This has been tested in a local Kubernetes cluster using a modified fork branch of helm/charts/stable/chaoskube where I turned on this new feature as CLI option. You can see this modification here.

An example of the structured output is here:
{"level":"info","msg":"terminating pod","name":"compose-api-6fbc44c575-mm47c","namespace":"docker","time":"2019-01-25T05:14:50Z"}

What's next?

If this gets merged, I'll raise a pull request in relevant helm chart with the modified fork branch mentioned earlier.

@linki
Copy link
Owner

linki commented Jan 28, 2019

@syedimam0012 Thank you! I like that.

How about we change the flag from a bool to a selection list with the current behaviour as the default. ExternalDNS uses a --log-format flag that can be either text or json (and possibly more in the future). I believe it simply configures the LogFormatter like you already do. It seems simpler and more extensible to me.

@syedimam0012
Copy link
Author

Thanks heaps for the kind response, @linki !

I agree that it makes more sense to update this to string options instead, which leaves room for future extensions. I do really appreciate for the example you've shared too.

The changes have just been committed upon testing all 3 possible cases that are default, text and json.

Please let me know if this requires further work.

I'll be happy to raise a relevant helm chart pr once this gets merged.

Best,
Syed

@linki
Copy link
Owner

linki commented Jan 29, 2019

Thanks for changing it.

If you use EnumVar instead of StringVar then kingpin will also do the validation for you. Any objection to using it?

@syedimam0012
Copy link
Author

syedimam0012 commented Jan 30, 2019

Pleasure and modified as advised, @linki!
Note: this has been tested in line with this helm repo pull-request. I'm yet to have those initial primary checks passed on that one.

syedimam0012 added 4 commits January 30, 2019 16:45
Signed-off-by: Syed Imam <syedimam0012@gmail.com>
Signed-off-by: Syed Imam <syedimam0012@gmail.com>
Signed-off-by: Syed Imam <syedimam0012@gmail.com>
Signed-off-by: Syed Imam <syedimam0012@gmail.com>
@linki linki merged commit 9dcc59e into linki:master Jan 30, 2019
@linki
Copy link
Owner

linki commented Jan 30, 2019

@syedimam0012 Thanks a lot!

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.

2 participants