Add additional logging to track mesh clients interacting with Namerd #2277
Conversation
Signed-off-by: Dennis Adjei-Baah <[email protected]>
Signed-off-by: Dennis Adjei-Baah <[email protected]>
Signed-off-by: Dennis Adjei-Baah <[email protected]>
Signed-off-by: Dennis Adjei-Baah <[email protected]>
Signed-off-by: Dennis Adjei-Baah <[email protected]>
Signed-off-by: Dennis Adjei-Baah <[email protected]>
dadjeibaah
force-pushed
the
dad/mesh-trace-logging
branch
from
8bb5269
to
44cf2f1
Compare
Signed-off-by: Dennis Adjei-Baah <[email protected]>
There was a problem hiding this comment.
Can you update the PR name and description to give some context and high level description of the approach here? What do you think about using the remote address (IP and ephemeral client port) in the server logs instead of generating a UUID client id? This would allow us to more easily associate log messages on the server with the client that initiated that request and would avoid the need for plumbing client ids all over the place.
| val frames = h2.Stream() | ||
| def loop(): Future[Unit] = | ||
| msgs.recv().transform { | ||
| case Return(Stream.Releasable(s, release)) => | ||
| log.trace(s"Streaming response metadata: ${reqMeta.mkString(":")}") |
There was a problem hiding this comment.
This says response metadata but it's printing the request headers, right?
There was a problem hiding this comment.
What's the intent here? Each time there's a response message, the request headers get printed?
There was a problem hiding this comment.
That's correct since the request headers may contain information useful for the particular request.
|
|
||
| class HeaderInjector(injectHeaders: Map[String, String]) extends SimpleFilter[Request, Response] { | ||
| override def apply(request: Request, service: Service[Request, Response]): Future[Response] = { | ||
| val req = request.dup() |
There was a problem hiding this comment.
is this necessary? aren't headers mutable?
There was a problem hiding this comment.
I remember from my last run-in with H2 request mangling that I had to dup() this for some reason, alas, I do not remember. I think we don't need this.
dadjeibaah
changed the title
|
Thanks for putting this together, @dadjeibaah. I think a simpler solution here would be for Namerd to simply include the remote (client) address in its log messages rather than generating a UUID on the client. The UUID approach has some problematic drawbacks:
I can see why a UUID solution would be desirable in specific situations where the client address is not available (such as when the request goes through a NAT) but I think that requirement is specialized enough that it doesn't merit putting this logic in master. |
There have been reports of Linkerd mesh clients not receiving updates from a Namerd server. There currently are watch state endpoints in Linkerd that tell you the last known value received from Namerd. This information is helpful in figuring out when Linkerd gets into this state. However, this information does not reveal the sequence of events that led up to that state.
This PR adds logging in the mesh client that logs every state change the Linkerd process is notified of i.e. Address set changes observed by Namerd. Given that this PR increases the number of log lines that will be available for diagnosis, each mesh client will now contain a unique identifier that will be injected when a watch request is initiated between Linkerd and Namerd for better analysis. This will help identify which client gets into a bad state when crawling through logs and will help in situations were remote IPs (NATed IPs) do not easily identify a Linkerd client in the Namerd logs.
fixes #2245