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

feat: add support for user-agent header #182

Merged
merged 1 commit into from
Sep 4, 2020

Conversation

gszr
Copy link
Contributor

@gszr gszr commented Aug 18, 2020

Given value is added to the user-agent header
set by the grpc-go library.

Related: grpc/grpc-go#255

cmd/grpcurl/grpcurl.go Outdated Show resolved Hide resolved
@@ -426,6 +430,9 @@ func main() {
} else if *authority != "" {
opts = append(opts, grpc.WithAuthority(*authority))
}
if *user_agent != "" {
opts = append(opts, grpc.WithUserAgent(*user_agent))
Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, neat. While setting user agent, I think it would be good to add grpcurl version info in here, too.

Maybe something like so:

grpcurlUA := sanitizeUA("grpcurl/" + version)
if version == "dev build <no version set>" {
    grpcurlUA = "grpcurl/dev-build (no version set)"
}
if *userAgent != "" {
    grpcurlUA = sanitizeUA(*userAgent) + " " + grpcurlUA
}
append(opts, grpc.WithUserAgent(grpcurlUA))

// sanitizeUA should replace whitespace chars (space, tab, CR, LF, vertical tab)
// with underscores. Ideally, it would be a little smarter and recognize if the UA
// is already in valid format -- <product>/<version> (<extra>) -- and only replace
// characters that need to be replaced to preserve this.

Would you be up for making such a change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jhump I've done a subset of the requested changes. The sanitization, however, I've skipped. Thinking a bit more about it, I feel that enforcing the format would be too restrictive; further, silently modifying the passed user-agent might be confusing. I can see use cases (e.g., testing) where that format is not followed. Thoughts?

@gszr gszr force-pushed the feat/user-agent-opt branch 2 times, most recently from 5d10f51 to ab18f1a Compare September 3, 2020 13:44
Copy link
Contributor

@jhump jhump left a comment

Choose a reason for hiding this comment

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

Looks good other than one tiny change needed.

cmd/grpcurl/grpcurl.go Show resolved Hide resolved
Given value is added to the user-agent header
set by the grpc-go library.
Copy link
Contributor

@jhump jhump 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 contribution!

@jhump jhump merged commit 9846afc into fullstorydev:master Sep 4, 2020
@gszr gszr deleted the feat/user-agent-opt branch September 4, 2020 15:37
@gszr
Copy link
Contributor Author

gszr commented Sep 4, 2020

Thanks for reviewing/merging, @jhump! Is there a timeline for the next release? Let me know if there's anything I can help with on it!

@gszr
Copy link
Contributor Author

gszr commented Oct 22, 2020

Hi @jhump - apologies to ask again, but is there a timeline for the next release? I'd be happy to help with it!

@jhump
Copy link
Contributor

jhump commented Jan 5, 2021

@gszr, I apologize for the long delays. The notice from GitHub from your last comment got buried in my inbox, so I never even saw it until today. I have now released v1.8.0, which includes this new flag. Sorry for the long lag between releases.

@gszr
Copy link
Contributor Author

gszr commented Jan 5, 2021

@jhump No worries! Thanks for this great tool and happy new year!

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

Successfully merging this pull request may close these issues.

2 participants