Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
rbasralian opened this issue Jul 1, 2024 · 8 comments
Assignees
Labels
feature request New feature or request grpc
Milestone

Comments

@rbasralian
Copy link
Contributor

rbasralian commented Jul 1, 2024

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.

@rbasralian rbasralian added feature request New feature or request triage labels Jul 1, 2024
@rcaudy rcaudy modified the milestones: June 2024, 0.36.0 Jul 1, 2024
@rcaudy rcaudy removed the triage label Jul 1, 2024
@niloc132 niloc132 changed the title Server should log version info of connecting client Clients should report a gRPC user-agent when connecting, servers should log it Jul 2, 2024
@niloc132 niloc132 added the grpc label Jul 2, 2024
@devinrsmith
Copy link
Member

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

@devinrsmith
Copy link
Member

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

@niloc132
Copy link
Member

niloc132 commented Jul 3, 2024

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
Copy link
Member

I'll add in barrage version as well.

@devinrsmith
Copy link
Member

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.

devinrsmith added a commit to devinrsmith/deephaven-core that referenced this issue Jul 10, 2024
This adds a structured, context-dependenent gRPC user-agent for the java client.

Here are some examples using the in-development 0.36.0-SNAPSHOT deephaven version:

SessionFactoryConfig client: `grpc-java/1.58.0 (deephaven/0.36.0-SNAPSHOT; deephaven-java-client-session)`

FlightSessionFactoryConfig client: `grpc-java/1.58.0 (deephaven/0.36.0-SNAPSHOT; flight/13.0.0; deephaven-java-client-flight)`

BarrageSessionFactoryConfig client: `grpc-java/1.58.0 (deephaven/0.36.0-SNAPSHOT; flight/13.0.0; barrage/0.6.0; deephaven-java-client-barrage)`

native application server client: `grpc-java/1.58.0 (deephaven/0.36.0-SNAPSHOT; flight/13.0.0; barrage/0.6.0; deephaven-server-jetty)`

python embedded server client: `grpc-java/1.58.0 (deephaven/0.36.0-SNAPSHOT; flight/13.0.0; barrage/0.6.0; deephaven-server-embedded)`

Integrators will be able to inherit the appropriate grpc-java, deephaven, flight, and barrage versions if they choose to, as well as the ability to append their own versions and contextual properties.

In addition, the flight.version was added to the default client.configuration.list configuration property.

Partial deephaven#5704
@rbasralian
Copy link
Contributor Author

rbasralian commented Jul 29, 2024

I had the Deephaven version in mind, but I certainly wouldn't object to more versions (e.g. gRPC version, Barrage version, and anything else that could be useful).

@niloc132
Copy link
Member

@rbasralian and while we could add a tool that just collects/shares the DH version, it seemed like a better bet to piggyback on the grpc version reporting feature, since we may not want to rely on adding more functionality to existing grpc/flight/etc clients - but they should support this, and in some cases might even make it possible to very lightly extend them to note the versions of any other pieces we add on top.

@alexpeters1208
Copy link
Contributor

Need @kosak to weigh in on what may be required to implement this in the R client, as it's backended by the C++ client, like ticking Python. R may or may not be blocked on C++.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request New feature or request grpc
Projects
None yet
Development

No branches or pull requests

6 participants