stats: add client side user agent to outgoing header #3331

piotrkowalczuk · 2020-01-23T10:00:22Z

This PR is a PoC and a proposal at the same time. It adds UserAgent field to stats.RPCTagInfo that carry the same information that is passed through metadata user-agent field. I have tried a similar approach with stats.ConnTagInfo however metadata is not yet available when TagConn is called on the server-side.

My main motivation to submit this change was to make a transition of my instrumentation library from Interceptors to stats.Handler possible. Since user-agent stores information about gRPC version and at the same time it may carry similar information about the client app name and its version, I think it might be very relevant information, e.g. to measure how newly deployed version behaves (canary deployment).

Related issue piotrkowalczuk/promgrpc#11

linux-foundation-easycla · 2020-01-23T10:00:25Z

The committers are authorized under a signed CLA.

✅ Piotr Kowalczuk (ee06009)

menghanl · 2020-01-23T18:07:16Z

On the server side, you should be able to get the metadata from context (metadata.FromIncomingContext(ctx)) and read the content.

On the client side, outgoing metadata doesn't have user-agent (it's appended later by the transport), so either this change, or we need to add it to the metadata.

In your interceptor based solution, how did you get user-agent on the client side?

dfawley · 2020-01-23T18:43:50Z

To get the gRPC version on this client side, you could use grpc.Version. Do you explicitly need the user agent string, even when it's overridden?

…context

piotrkowalczuk · 2020-01-23T19:00:08Z

On the client side, outgoing metadata doesn't have user-agent (it's appended later by the transport), so either this change, or we need to add it to the metadata.

I like the idea of passing the data using an already existing mechanism.

In your interceptor based solution, how did you get user-agent on the client side?

I haven't, as you described metadata is not available.

To get the gRPC version on this client side, you could use grpc.Version. Do you explicitly need the user agent string, even when it's overridden?

Especially when it is overridden. From an application maintainer point of view, I expect to change my code more frequently than gRPC version, so user-agent section that describes my own app is more interesting to me. Having gRPC version included is useful as well though.

menghanl · 2020-01-23T22:11:30Z

With a second thought:
user-agent is kind of at a grey area. It's not a gRPC concept, but an http/http2 concept. So if the transport is changed to another protocol, user-agent wouldn't be appropriate.
This is the main reason user-agent is not set in client metadata, but is set by the client transport.

For the same reason, I would prefer to keep the transport specific details independent from the APIs gRPC exposes. (Though we are already violating that by having a dial option to set user agent, but that's more for historical reasons, and a better way that I would do today is to have a generic dial option to set transport configs that only a specific transport can understand)

Adding user-agent to outgoing metadata at the gRPC layer would break the layers.

Will you need user-agent on the client side?
You mentioned that you didn't have it in the interceptor based solution. I also think it's less useful comparing to the server side.

piotrkowalczuk · 2020-01-23T23:11:41Z

Will you need user-agent on the client side?

The main reason to have such value on both sides is to be able to match metrics with each other or being able to distinguish metrics (e.g. number of outgoing requests) produced by different version of an app. Something like this might be done on the server-side, however:

it is not guaranteed that app A will always talk to the app B
if contract B implemented in team B by app B would one day be implemented by app B2 it might happen that dashboard/alerts of app A in team A would break. In other words I would prefer to keep metrics of team B private.

I completely get your point, and it is clear to me that this is not the best way to address my problem. Do you consider to introduce any other mechanism that would serve a similar purpose?

menghanl · 2020-01-24T20:25:24Z

Another option is to add user-agent to OutHeader. Today, it's only a copy of the metadata, so it doesn't have user-agent.
We can manually append it. (There are other fields sent by the client, but not in OutHeader. I think it should be OK to leave these alone for now, until someone asks for them. And some of them are oauth headers that probably shouldn't be exposed).

This means you won't get the info in TagRPC, but will later in HandleRPC.

dfawley · 2020-01-24T21:05:53Z

For the same reason, I would prefer to keep the transport specific details independent from the APIs gRPC exposes

I would say that some of the stats API already contains HTTP-transport-specific details (e.g. wire length). So adding something transport-specific to that API is not out of the question IMO. However, we should be careful about how we expose it.

Adding user-agent to outgoing metadata at the gRPC layer would break the layers.

I agree with this. If we add it, it should be done inside the transport instead (and include the full header value, matching what would be seen by the server).

piotrkowalczuk · 2020-01-24T22:36:08Z

This means you won't get the info in TagRPC, but will later in HandleRPC.

A problem I can see with this approach is that such metadata would not be available in all HandleRPC calls.*stats.OutHeader fires after *stats.Begin.

menghanl · 2020-01-28T22:19:07Z

The timing problem is a little tricky.
On the client side, TagRPC, HandleRPC Begin, and HandleRPC OutHeader should all happen about the same time.

Would it be OK to leave user-agent empty, and populate it later while handling OutHeader? Or even log the begin while handling OutHeader? This should be early enough for both unary and streaming RPCs.

piotrkowalczuk · 2020-01-28T23:18:03Z

@menghanl makes sense. Done.

…metadata produced on the client side

menghanl

Thanks for the fix.

Seems vet isn't happy for some reason (travis log doesn't show the actually error...)
Looks like it failed gofmt -s -d -l .. Try it locally and see if there's any diff? You can also just run vet.sh.

stats/stats_test.go

internal/transport/http2_client.go

menghanl · 2020-01-30T22:10:43Z

internal/transport/http2_client.go

@@ -681,13 +681,15 @@ func (t *http2Client) NewStream(ctx context.Context, callHdr *CallHdr) (_ *Strea
 	}
 	if t.statsHandler != nil {
 		header, _, _ := metadata.FromOutgoingContextRaw(ctx)


Actually, I believe this is a bug to call Raw here.
This should be just FromOutgoingContext(), otherwise it will miss the metadata that's appended.
Can you fix it, too? Thanks!

And I believe if you change this, the copy() won't be necessary anymore, because FromOutgoingContext always does a join. (The comment of the function also needs update, but that's another issue).

…NewStream

menghanl

Thanks for all the changes! LGTM.

stats handler access to client user agent

ee06009

piotrkowalczuk added 3 commits January 23, 2020 19:32

stats handler access to client user agent through metadata

b4078ee

code formatting cleanup

8e80ee3

code formatting cleanup grpc#2

13932c2

NewOutgoingContext grouped with the rest of the code that manipulate …

893bbf4

…context

piotrkowalczuk added 4 commits January 23, 2020 20:42

Use AppendToOutgoingContext instead of NewOutgoingContext

89596fa

travis force rebuild

651a72e

travis force rebuild 2

953aaeb

gofmt fix

fbaabed

dfawley requested a review from menghanl January 23, 2020 21:26

dfawley assigned menghanl Jan 23, 2020

headers passed into stats.OutHeader gets user-agent header as well

70d9218

stats - testMetadata new entry, user-agent expected to be present in …

6439d9b

…metadata produced on the client side

menghanl suggested changes Jan 29, 2020

View reviewed changes

stats/stats_test.go Outdated Show resolved Hide resolved

menghanl reviewed Jan 29, 2020

View reviewed changes

internal/transport/http2_client.go Outdated Show resolved Hide resolved

menghanl added the Type: Feature New features or improvements in behavior label Jan 29, 2020

menghanl added this to the 1.28 Release milestone Jan 29, 2020

piotrkowalczuk added 2 commits January 29, 2020 13:31

review fixes

555e972

review fixes grpc#2

30ceba8

menghanl reviewed Jan 30, 2020

View reviewed changes

FromOutgoingContext instead of FromOutgoingContextRaw in http2Client.…

799355f

…NewStream

menghanl approved these changes Jan 30, 2020

View reviewed changes

menghanl changed the title ~~stats.Handler access to client user agent through stats.RPCTagInfo~~ stats: add client side user agent to outgoing header Jan 31, 2020

menghanl merged commit 7621679 into grpc:master Jan 31, 2020

github-actions bot locked as resolved and limited conversation to collaborators May 25, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

stats: add client side user agent to outgoing header #3331

stats: add client side user agent to outgoing header #3331

piotrkowalczuk commented Jan 23, 2020 •

edited

Loading

linux-foundation-easycla bot commented Jan 23, 2020 •

edited

Loading

menghanl commented Jan 23, 2020

dfawley commented Jan 23, 2020

piotrkowalczuk commented Jan 23, 2020 •

edited

Loading

menghanl commented Jan 23, 2020

piotrkowalczuk commented Jan 23, 2020 •

edited

Loading

menghanl commented Jan 24, 2020

dfawley commented Jan 24, 2020

piotrkowalczuk commented Jan 24, 2020

menghanl commented Jan 28, 2020

piotrkowalczuk commented Jan 28, 2020 •

edited

Loading

menghanl left a comment

menghanl Jan 30, 2020

piotrkowalczuk Jan 30, 2020

menghanl left a comment

stats: add client side user agent to outgoing header #3331

stats: add client side user agent to outgoing header #3331

Conversation

piotrkowalczuk commented Jan 23, 2020 • edited Loading

linux-foundation-easycla bot commented Jan 23, 2020 • edited Loading

menghanl commented Jan 23, 2020

dfawley commented Jan 23, 2020

piotrkowalczuk commented Jan 23, 2020 • edited Loading

menghanl commented Jan 23, 2020

piotrkowalczuk commented Jan 23, 2020 • edited Loading

menghanl commented Jan 24, 2020

dfawley commented Jan 24, 2020

piotrkowalczuk commented Jan 24, 2020

menghanl commented Jan 28, 2020

piotrkowalczuk commented Jan 28, 2020 • edited Loading

menghanl left a comment

Choose a reason for hiding this comment

menghanl Jan 30, 2020

Choose a reason for hiding this comment

piotrkowalczuk Jan 30, 2020

Choose a reason for hiding this comment

menghanl left a comment

Choose a reason for hiding this comment

piotrkowalczuk commented Jan 23, 2020 •

edited

Loading

linux-foundation-easycla bot commented Jan 23, 2020 •

edited

Loading

piotrkowalczuk commented Jan 23, 2020 •

edited

Loading

piotrkowalczuk commented Jan 23, 2020 •

edited

Loading

piotrkowalczuk commented Jan 28, 2020 •

edited

Loading