search

strimzi / strimzi-canary

Strimzi canary
Apache License 2.0
42 stars 28 forks source link

include milliseconds is sarama debug log timestamp #189

Closed k-wall closed 2 years ago

k-wall commented 2 years ago

Sarama debug logging is made more useful by including the milliseconds portion in the timestamps. With this change the output looks like:

[Sarama] 2022/06/08 19:13:47.953676 client/brokers registered new broker #0 at broker-0-penguin-cagbmplvo-vi--h--aog.kas.kwall-kafka.3zz3.s1.devshift.org:443
[Sarama] 2022/06/08 19:13:47.953693 Successfully initialized new client
k-wall commented 2 years ago

Mmmm.. I'm sure I replied to this already but it seems to have vanished into the ether.

I'm happy to add configurability (an environment variable to get back the original behaviour is probably the easiest). However I think the likelihood of any user actually being negatively impacted by this change is tiny. kubelet adds timestamps information to each line of output produced by a container, so I think any integration that wanted the log timestamps would take that source over parsing timestamp information from the raw canary log. If I'd remembered fact that kubectl logs --timestamps=true existed during my debugging, I probably wouldn't have even proposed the change.

I don't feel strongly between merging as-is, adding configurability, or closing the PR unactioned.

scholzj commented 2 years ago

I'm not sure how kubelet or kubectl relate to this. I don§t think they are how most users consume the logs. But ok, if we do not expect any problems, we should go with it. I guess it is always easier to add it later if needed, but in we probably also don't want to carry it long term, so mayne even if it causes issue to someone we should maybe go through it.