Skip to content

Conversation

@ikait
Copy link
Contributor

@ikait ikait commented Jun 17, 2020

This pull request adds a userAgent option to CallOptions struct. It makes clients possible to request with customized user-agent to the gRPC servers which require the particular format of user-agent.

Currently it partially supports customize user-agent by setting customMetadata with "user-agent" key but it always produces string starts with "grpc-swift-nio, ".

In a project I belong to have a log parser that requires a particular user-agent and couldn't accept string having that prefix. I think other gRPC clients can be set custom user-agent so that it's also useful for grpc-swift.

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Jun 17, 2020

CLA Check
The committers are authorized under a signed CLA.

@glbrntt glbrntt self-requested a review June 17, 2020 13:55
Copy link
Collaborator

@glbrntt glbrntt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the pull request @ikait; however, I think the proposed change doesn't fix the original problem you faced though: setting the user-agent in customMetadata will still be added rather than replacing the existing user-agent.

To that end, I think the fix is actually much simpler than you propose: we should only add the "standard" user-agent string if the user didn't provide one in customMetadata.

It may also be useful if users could set the user-agent at the connection level as well as at the RPC level (via customMetadata). Setting it at the connection level would simply override the current "grpc-swift-nio" value. I'm happy to leave this as an additional task though.

@glbrntt glbrntt added kind/enhancement Improvements to existing feature. nio labels Jun 17, 2020
@ikait ikait changed the title Add a userAgent option to CallOptions struct Add default user-agent to header if customMetadata doesn't have one Jun 17, 2020
@ikait
Copy link
Contributor Author

ikait commented Jun 17, 2020

Thank you for the comment, @glbrntt. I saw your points and rewrote my proposal with much simpler changes. The idea that setting the user-agent at the connection level is looks good but does it need much more changes than ones with RPC level? At this pull request I would like to address my original problem with RPC level fix.

@ikait ikait requested a review from glbrntt June 17, 2020 15:45
@glbrntt glbrntt added the 🔨 semver/patch No public API change. label Jun 17, 2020
Copy link
Collaborator

@glbrntt glbrntt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @ikait, looks great!

@glbrntt
Copy link
Collaborator

glbrntt commented Jun 17, 2020

The idea that setting the user-agent at the connection level is looks good but does it need much more changes than ones with RPC level? At this pull request I would like to address my original problem with RPC level fix.

That's fine with me, I'll create an issue to capture this information, there's no need to do it now :)

@glbrntt glbrntt merged commit c91666e into grpc:master Jun 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

kind/enhancement Improvements to existing feature. 🔨 semver/patch No public API change.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants