search

deephaven / deephaven-core

Deephaven Community Core
Other
236 stars 79 forks source link

Clients should report a gRPC user-agent when connecting, servers should log it #5704

Open rbasralian opened 6 days ago

rbasralian commented 6 days ago

When a client connects, the server should log the client's version number. (This will be very helpful for debugging, especially in environments with multiple clients who may not have pulled the most up-to-date client package.)

From https://github.com/grpc/grpc/blob/master/doc/PROTOCOL-HTTP2.md#user-agents

While the protocol does not require a user-agent to function it is recommended that clients provide a structured user-agent string that provides a basic description of the calling library, version & platform to facilitate issue diagnosis in heterogeneous environments. The following structure is recommended to library developers

User-Agent → "grpc-" Language ?("-" Variant) "/" Version ?( " ("  *(AdditionalProperty ";") ")" )

E.g.

grpc-java/1.2.3
grpc-ruby/1.2.3
grpc-ruby-jruby/1.3.4
grpc-java-android/0.9.1 (gingerbread/1.2.4; nexus5; tmobile)

Each Deephaven client implementation should follow this, probably specifying their grpc version and DH version, and potentially related dependencies (python/c++ should probably include the flight version).

Then, the server should capture this value, at least for error messages logged on the server, and potentially store it in the session (assuming that session tokens are not reused across multiple clients).

Note that the Java client already has wiring to enable it to define a user agent, but at this time only io.deephaven.client.examples.ConnectOptions actually is capable of setting it. The Java FlightClient does not set it either.

devinrsmith commented 4 days ago

Is "Version" here meant to represent the grpc version, or the Deephaven version? We should be internally consistent among our different languages.

devinrsmith commented 4 days ago

I'm assuming something like grpc-java/<grpc-version> (deephaven/<deephaven-version>) would be what we want for the java client?

niloc132 commented 4 days ago

I'm assuming something like grpc-java/<grpc-version> (deephaven/<deephaven-version>) would be what we want for the java client?

Yes exactly - the JS client wouldn't list a grpc version (since it doesn't use a grpc client at all), and other clients that leverage arrow flight more directly should also be sure to list that version as well.

Two subtasks that I'll edit in (along with a point per client) after any further discussion: server needs to make some use of these strings, and the grpc-web filter needs to read from x-user-agent instead (see "User Agent" at https://github.com/grpc/grpc/blob/master/doc/PROTOCOL-WEB.md#protocol-differences-vs-grpc-over-http2).

devinrsmith commented 4 days ago

I'll add in barrage version as well.

devinrsmith commented 4 days ago

From the perspective of "worker to worker", it may also be good to have a property describing how the server was packaged, ie "native", "py-embedded-server", "dnd", etc.