use slogger on local server, add span_id id to logs, add kolide session id to logs #1446
Conversation
| return | ||
| } | ||
|
|
||
| if resp != nil && resp.Body != nil { | ||
| resp.Body.Close() | ||
| } | ||
|
|
||
| level.Debug(e.logger).Log("msg", "Finished callback", "response-status", resp.Status) | ||
| e.slogger.Log(req.Context(), slog.LevelDebug, "finished callback", | ||
| "response-status", resp.Status, |
There was a problem hiding this comment.
| "response-status", resp.Status, | |
| "response_status", resp.Status, |
| kolideSessionId, ok := cmdReq.CallbackHeaders[multislogger.KolideSessionIdKey.String()] | ||
| if ok && len(kolideSessionId) > 0 { | ||
| span.SetAttributes(attribute.String(multislogger.KolideSessionIdKey.String(), kolideSessionId[0])) | ||
| r = r.WithContext(context.WithValue(r.Context(), multislogger.KolideSessionIdKey, kolideSessionId[0])) |
There was a problem hiding this comment.
This line feels awkward. It's pulling in request, context, and multislogger to stash a session key in the request context. I suspect it's correct, I don't know enough to know how to make it cleaner. But it feels awkward.
There was a problem hiding this comment.
I think it would be less awkward if the expected header name was a constant here, it should not overload multislogger.KolideSessionIdKey. That brings in unneeded coupling
There was a problem hiding this comment.
I've updated it so that we define the header key here as a constant. However, we still use the contextKey iota in multislogger to set context value
There was a problem hiding this comment.
Yah. That part feels awkward, but I don't know enough to know how to fix it. We are, ultimately, trying to set internal state in context that multislogger uses. So it has to come from there.
But we should not stash http headers there
| w.WriteHeader(http.StatusUnauthorized) | ||
| return | ||
| } | ||
|
|
||
| // set the kolide session id if it exists, this also the saml session id |
There was a problem hiding this comment.
I'm not sure kolide session id and saml session id are the same.
There was a problem hiding this comment.
I ask Chris and he confirmed it was the saml id
There was a problem hiding this comment.
They might be the same now, but I'm not sure they'll be the same later. I'm suggesting we drop the comment about saml session id.
pkg/log/multislogger/multislogger.go
Outdated
| // KolideSessionIdKey this the also the saml session id | ||
| KolideSessionIdKey contextKey = "X-Kolide-Session" | ||
| SpanIdKey contextKey = "span_id" |
There was a problem hiding this comment.
This feels wrong. If these are context keys, they should probably be iotas. I don't think we should encode the header name here.
There was a problem hiding this comment.
now using iota, now the string of KolideSessionIdKey is kolide_session_id
This PR: