feat: log session ID and user-agent #5994
Conversation
Logs the session ID and user-agent (if set) on session creation. Technically, the user-agent could change with each RPC call, but that is not something that clients typically do. For contexts where logging every session is too spammy, the configuration property `SessionListenerLogger.level` can be used to set the appropriate log level. Note, the user agent interceptor is not able to be bound to `Set<ServerInterceptor>` as it causes an undefined ordering with respect to `SessionServiceInterceptor`; we need to ensure that the user agent interceptor is applied before the session service interceptor. Server component of deephaven#5704
devinrsmith
added
logging
NoDocumentationNeeded
ReleaseNotesNeeded
|
Here's an example of connecting from a browser: Here's an example of connect-check: |
| @Inject | ||
| public SessionListenerLogger() { | ||
| level = LogLevel.valueOf(Configuration.getInstance().getStringForClassWithDefault(SessionListenerLogger.class, | ||
| "level", LogLevel.INFO.getName())); |
There was a problem hiding this comment.
Why make this configurable, instead of the usual approach of "change the class's log level"? There is only one log message in this class, so enabling/disabling the log level seems like it should do it.
Frustrating to find that this can only be configured from the xml file, at least with our current setup...
There was a problem hiding this comment.
I'm also not sure that this is enough to really address the class of issues we were seeing - we only will see it on connection, rather than paired with the thing that went wrong.
It isn't quite right to pair the authenticated user agent with the exec/auth context, but it would be closer to attach the current request's auth context to the exec context so we can see it for a given grpc call, and dump it in case of an error?
There was a problem hiding this comment.
Why make this configurable
It's mostly just the (lack of) configurability of the logging XML; instead of doing this, I could just make it debug, or removal it altogether.
It isn't quite right to pair the authenticated user agent with the exec/auth context, but it would be closer to attach the current request's auth context to the exec context so we can see it for a given grpc call, and dump it in case of an error?
I might instead suggest that our error logging sites just call and log UserAgentContext.get() directly. In that way, we don't need to attach anything to the auth context.
What was the specific error we were seeing where user-agent would have been helpful?
|
|
||
| public final class UserAgentContext { | ||
|
|
||
| private static final String USER_AGENT_HEADER = "user-agent"; |
There was a problem hiding this comment.
Just a heads up that this is going to seem inconsistent for now from browsers - using h2/https, browsers will report their plain user-agent header, while using http (and websockets) this will be empty. Neither is correct for grpc - the user-agent header is not correct to read for grpc-web calls, since browsers do not permit web applications to write this (so we can't get our own versions in there). Instead, clients must set x-user-agent, and our GrpcWebFilter most replace this.
There was a problem hiding this comment.
It's probably useful for our server to get the browser user agents, at least in the context of h2. Seems like we might want to unconditionally attach x-user-agent info too, or are you saying that GrpcWebFilter should take the value from x-user-agent and shove it into user-agent?
2 participants
Logs the session ID and user-agent (if set) on session creation. Technically, the user-agent could change with each RPC call, but that is not something that clients typically do. For contexts where logging every session is too spammy, the configuration property
SessionListenerLogger.levelcan be used to set the appropriate log level.Note, the user agent interceptor is not able to be bound to
Set<ServerInterceptor>as it causes an undefined ordering with respect toSessionServiceInterceptor; we need to ensure that the user agent interceptor is applied before the session service interceptor.Server component of #5704