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

Include level in JSON log messages (useful for automatic status mapping in DataDog) #1426

Open
cbugneac-nex opened this issue Feb 19, 2024 · 8 comments
Assignees
Labels
kind/feature Categorizes issue or PR as related to a new feature. triage/accepted Indicates an issue or PR is ready to be actively worked on.

Comments

@cbugneac-nex
Copy link

cbugneac-nex commented Feb 19, 2024

What would you like to be added:
Include the level in the JSON log messages.

Why is this needed:
So that it's clear what type of log messages are these, e.g info, warning, error, etc.
At the moment these JSON log messages:

{"ts":1708343903247.908,"caller":"options/serving.go:374","msg":"Generated self-signed cert (/tmp/apiserver.crt, /tmp/apiserver.key)","v":0}                                                                                   │
{"ts":1708343906442.6155,"caller":"aggregated/handler.go:275","msg":"Adding GroupVersion metrics.k8s.io v1beta1 to ResourceManager","v":0}                                                                                     │
{"ts":1708343906652.7542,"caller":"server/secure_serving.go:213","msg":"Serving securely on :10250","v":0}                                                                                                                     │
{"ts":1708343906652.7927,"caller":"headerrequest/requestheader_controller.go:169","msg":"Starting RequestHeaderAuthRequestController","v":0}                                                                                   │
{"ts":1708343906652.8008,"caller":"cache/shared_informer.go:311","msg":"Waiting for caches to sync for RequestHeaderAuthRequestController","v":0}                                                                              │
{"ts":1708343906652.823,"caller":"dynamiccertificates/dynamic_serving_content.go:132","msg":"Starting controller","v":0,"name":"serving-cert::/tmp/apiserver.crt::/tmp/apiserver.key"}                                         │
{"ts":1708343906652.873,"caller":"dynamiccertificates/tlsconfig.go:240","msg":"Starting DynamicServingCertificateController","v":0}                                                                                            │
{"ts":1708343906652.9602,"caller":"dynamiccertificates/configmap_cafile_content.go:202","msg":"Starting controller","v":0,"name":"client-ca::kube-system::extension-apiserver-authentication::client-ca-file"}                 │
{"ts":1708343906738.6284,"caller":"cache/shared_informer.go:311","msg":"Waiting for caches to sync for client-ca::kube-system::extension-apiserver-authentication::client-ca-file","v":0}                                      │
{"ts":1708343906738.739,"caller":"dynamiccertificates/configmap_cafile_content.go:202","msg":"Starting controller","v":0,"name":"client-ca::kube-system::extension-apiserver-authentication::requestheader-client-ca-file"}    │
{"ts":1708343906738.7761,"caller":"cache/shared_informer.go:311","msg":"Waiting for caches to sync for client-ca::kube-system::extension-apiserver-authentication::requestheader-client-ca-file","v":0}                        │
{"ts":1708343906838.8875,"caller":"cache/shared_informer.go:318","msg":"Caches are synced for client-ca::kube-system::extension-apiserver-authentication::client-ca-file","v":0}                                               │
{"ts":1708343906838.9417,"caller":"cache/shared_informer.go:318","msg":"Caches are synced for client-ca::kube-system::extension-apiserver-authentication::requestheader-client-ca-file","v":0}                                 │
{"ts":1708343906839.1077,"caller":"cache/shared_informer.go:318","msg":"Caches are synced for RequestHeaderAuthRequestController","v":0}

are detected in DataDog as errors. Adding level would allow automatic mapping to correct status.
image

Used args:

            args:
              - --secure-port=10250
              - --cert-dir=/tmp
              - --kubelet-preferred-address-types=InternalIP,ExternalIP,Hostname
              - --kubelet-use-node-status-port
              - --metric-resolution=15s
              - --logging-format=json

/kind feature

@k8s-ci-robot k8s-ci-robot added kind/feature Categorizes issue or PR as related to a new feature. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Feb 19, 2024
@cbugneac-nex cbugneac-nex changed the title Include level in JSON log messages for automatic status mapping in DataDog Include level in JSON log messages (useful for automatic status mapping in DataDog) Feb 19, 2024
@dashpole
Copy link

/assign @dgrisonnet
/triage accepted

@k8s-ci-robot k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Feb 22, 2024
@yashvardhan-kukreja
Copy link

yashvardhan-kukreja commented Mar 25, 2024

Hi folks, I looked a bit into this issue.
This would be easy to solve but seems to be impossible to solve from metrics-server.
That's because the inclusion of level field is actually a configurable attribute of the zapr logger library used under-the-hood.
But metrics-server doesn't use it directly, instead it uses the k8s.io/component-base as a wrapper to initialize and apply logging configurations and the whole zapr configuration and implementation lies under component-base instead.

The sad part is that k8s.io/component-base doesn't expose relevant options to configure the level key and as of now, it just omits the "level" key when initializing the JSON logger as a default case irrespective of what the invoker/client (for eg: metrics-server) wants.

References:

What could be the fix then?

  • Smallest one, yet not so clean one: Modify the component-base's NewJSONLogger to include LevelKey as well in its default EncoderConfig i.e. the following two lines of code to be added here
		LevelKey:       "level",
		EncodeLevel:    zapcore.LowercaseLevelEncoder,

Working example

  • Larger but cleaner and extensible one: Modify component-base to allow clients like metrics-server to provide more granular options like providing EncoderConfig for the underlying JSON logger to use instead of blatantly using nil (causing it to forcefully use the default encoderconfig)

@yashvardhan-kukreja
Copy link

Update: I have raised an issue in component-base to address this issue as well as linked just before this comment

@yashvardhan-kukreja
Copy link

@dashpole @dgrisonnet would you folks mind re-assigning this issue to me, just considering no updates on before my comments?

@dashpole
Copy link

I believe the component-base repo is synced from kubernetes/kubernetes/staging. I would open an issue in k/k itself

@yashvardhan-kukreja
Copy link

I believe the component-base repo is synced from kubernetes/kubernetes/staging. I would open an issue in k/k itself

Sure @dashpole

Would you mind assigning me the issue when you open that issue considering I have already navigated the component-base fairly enough to realise where the changes would potentially be needed to be made.

Would love to contribute on this one!

@gmarcot
Copy link

gmarcot commented May 21, 2024

Hello folks,

any news about this request?

Many thanks for your support

@jnogol
Copy link

jnogol commented Jul 8, 2024

this would be very useful

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature Categorizes issue or PR as related to a new feature. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
None yet
Development

No branches or pull requests

7 participants