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

Set User-Agent header on both REST and graphql requests #259

Merged
merged 1 commit into from
Apr 20, 2023

Conversation

yob
Copy link
Contributor

@yob yob commented Apr 18, 2023

In #256 we starting setting a custom User-Agent header on REST API calls, to help us understand the ways our API is used. It looks like this:

Terraform/ (+https://www.terraform.io) Terraform-Plugin-SDK/2.0.3 buildkite/0.15.0

This builds on that work and sets the same User-Agent on graphql API requests as well.

It drops the unnecessary use of golang.org/x/oauth2 to set a static bearer token header, and uses the tripperware pattern to set both headers we care about:

  1. the static bearer token for auth
  2. the user agent

The same http.Client is used for both REST and graphql, so we'll get identical behaviour on both APIs.

@yob yob force-pushed the graphql-user-agent branch from c9bf2d5 to 49ca961 Compare April 18, 2023 11:23
@yob yob requested review from danstn and mcncl April 18, 2023 11:26
@yob
Copy link
Contributor Author

yob commented Apr 18, 2023

@jradtilbrook @mcncl absolutely no hurry on this one, I just thought I'd poke at it while the changes in #256 were fresh in our brains. If we merge it, it could wait for the next release you do for other reasons.

danstn
danstn previously approved these changes Apr 20, 2023
Copy link
Contributor

@danstn danstn left a comment

Choose a reason for hiding this comment

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

Great idea @yob!

There are a few minor comments, but I think it's safe to skip these and add https://github.com/golangci/golangci-lint in a separate PR instead.

buildkite/client.go Outdated Show resolved Hide resolved
buildkite/client.go Outdated Show resolved Hide resolved
buildkite/client.go Outdated Show resolved Hide resolved
req.Header[k] = v
}
}
return rt.next.RoundTrip(req)
Copy link
Contributor

Choose a reason for hiding this comment

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

RoundTrip returns an error, so normally we'd want to wrap it to provide additional context.

Copy link
Contributor Author

@yob yob Apr 20, 2023

Choose a reason for hiding this comment

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

I've addressed the other feedback, but left this one. I could wrap the error, but... I really wasn't sure what to add that was meaningful.

@mcncl
Copy link
Contributor

mcncl commented Apr 20, 2023

Happy to merge once @danstn suggestions are committed in. Also happy to just commit them from comment too.

@danstn
Copy link
Contributor

danstn commented Apr 20, 2023

Happy to merge once @danstn suggestions are committed in. Also happy to just commit them from comment too.

Oh these are not blocking, happy to land it as is. @yob up to you

@yob
Copy link
Contributor Author

yob commented Apr 20, 2023

They're good suggestions, I'll make them in a bit ❤️

In #256 we starting setting a custom User-Agent header on REST API
calls, to help us understand the ways our API is used. It looks like this:

    Terraform/ (+https://www.terraform.io) Terraform-Plugin-SDK/2.0.3 buildkite/0.15.0

This builds on that work and sets the same User-Agent on graphql API
requests as well.

It drops the unnecessary use of golang.org/x/oauth2 to set a static bearer
token header, and uses the tripperware pattern [1] to set both headers we
care about:

1. the static bearer token for auth
2. the user agent

The same http.Client is used for both REST and graphql, so we'll get
identical behaviour on both APIs.

[1] https://dev.to/stevenacoffman/tripperwares-http-client-middleware-chaining-roundtrippers-3o00
Copy link
Contributor

@mcncl mcncl left a comment

Choose a reason for hiding this comment

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

LGTM

@yob
Copy link
Contributor Author

yob commented Apr 20, 2023

I've confirmed that we see the Terraform useragent in our API access logs for both the REST and graphql calls

@yob yob merged commit ae36428 into main Apr 20, 2023
@yob yob deleted the graphql-user-agent branch April 20, 2023 06:37
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.

3 participants