Add tracing for incoming requests #114
Conversation
Note that Kubernetes API calls made here are not traced because they are not made using the context with the span attached.
…s in prepare downscale webhook
| oldObj, oldGVK, err := codecs.UniversalDeserializer().Decode(ar.Request.OldObject.Raw, nil, nil) | ||
| if err != nil { | ||
| return allowErr(logger, "can't decode old object, allowing the change", err) | ||
| } | ||
| logger = log.With(logger, "request_gvk", oldGVK) |
There was a problem hiding this comment.
@56quarters commented in #101:
I appreciate trying to minimize changes but it seems weird to use both
loggerandspanLogger. Can we use just thespanLoggerand not keeploggeraround in these methods?
There was a problem hiding this comment.
I kept both to preserve the behaviour of tagging log lines with the attributes added along the way such as request_gvk. The trouble comes from log.With returning a log.Logger, rather than a spanlogger.SpanLogger - so as soon as we call log.With, we lose the ability to set span tags.
One option would be to add another method to SpanLogger that allows us to add a tag and wrap the underlying logger with another attribute, which would tidy this up - what do you think?
There was a problem hiding this comment.
That sounds reasonable but let's not block this PR on it. Thanks 👍
There was a problem hiding this comment.
grafana/dskit#467 will add the method needed for this to dskit.
There was a problem hiding this comment.
...and #115 will use that method to tidy this up
| oldObj, oldGVK, err := codecs.UniversalDeserializer().Decode(ar.Request.OldObject.Raw, nil, nil) | ||
| if err != nil { | ||
| return allowErr(logger, "can't decode old object, allowing the change", err) | ||
| } | ||
| logger = log.With(logger, "request_gvk", oldGVK) |
There was a problem hiding this comment.
That sounds reasonable but let's not block this PR on it. Thanks 👍
Continuation of #101, which was merged and then reverted due to some feedback received after merging.