cmd/scollector: Set custom UserAgent and use facebookgo/httpcontrol for 60s RequestTimeout #1878

Merged
merged 3 commits into from Aug 25, 2016

Conversation

Projects
None yet
2 participants
@gbrayut
Contributor

gbrayut commented Aug 20, 2016

No description provided.

@gbrayut

This comment has been minimized.

Show comment
Hide comment
@gbrayut

gbrayut Aug 20, 2016

Contributor

Not ready to merge yet, as I plan on testing on a few systems for a while first.

Contributor

gbrayut commented Aug 20, 2016

Not ready to merge yet, as I plan on testing on a few systems for a while first.

@gbrayut gbrayut removed the WIP label Aug 24, 2016

@gbrayut

This comment has been minimized.

Show comment
Hide comment
@gbrayut

gbrayut Aug 24, 2016

Contributor

This is working now and ready for review. I am going to test it on a few systems to make sure the new 60s timeout doesn't cause any issues.

Fixes #1879

Contributor

gbrayut commented Aug 24, 2016

This is working now and ready for review. I am going to test it on a few systems to make sure the new 60s timeout doesn't cause any issues.

Fixes #1879

@gbrayut

This comment has been minimized.

Show comment
Hide comment
@gbrayut

gbrayut Aug 24, 2016

Contributor

I added another commit that will set the UA for the collect package when used in tsdbrelay and bosun. I'll also test these on my VM.

Contributor

gbrayut commented Aug 24, 2016

I added another commit that will set the UA for the collect package when used in tsdbrelay and bosun. I'll also test these on my VM.

collect/collect.go
@@ -47,6 +47,9 @@ var (
// Whether or not to use NTLM authentication
UseNtlm bool = false
+ // DefaultClient can be used to override the HTTP client that will be used to make requests.
+ DefaultClient *http.Client

This comment has been minimized.

@captncraig

captncraig Aug 25, 2016

Contributor

This is kinda wierd to me. Collect already has an odd client with a ghetto timeout thing (that looks buggy to me). Can we just have a central utility that sets http.DefaultClient?

Something like util.SetupHttpClient(appName, msg, features...)?

@captncraig

captncraig Aug 25, 2016

Contributor

This is kinda wierd to me. Collect already has an odd client with a ghetto timeout thing (that looks buggy to me). Can we just have a central utility that sets http.DefaultClient?

Something like util.SetupHttpClient(appName, msg, features...)?

This comment has been minimized.

@captncraig

captncraig Aug 25, 2016

Contributor

I'd rather collect never had to know anything about http clients, and can just Get and Post to its heart's content.

@captncraig

captncraig Aug 25, 2016

Contributor

I'd rather collect never had to know anything about http clients, and can just Get and Post to its heart's content.

cmd/scollector/conf/conf.go
+ // UserAgentMessage is an optional message that will be appended to the
+ // end of the User Agent when making HTTP requests.
+ // Example: Scollector/0.6.0-pre (Custom message here)
+ UserAgentMessage string

This comment has been minimized.

@captncraig

captncraig Aug 25, 2016

Contributor

This is a doc change. Is the primary use case here putting the hostname? Might want to note why this exists in the docs.

@captncraig

captncraig Aug 25, 2016

Contributor

This is a doc change. Is the primary use case here putting the hostname? Might want to note why this exists in the docs.

@captncraig

This comment has been minimized.

Show comment
Hide comment
@captncraig

captncraig Aug 25, 2016

Contributor

In general, it looks pretty good. I'd just like to reduce some of the duplicated code if possible.

Contributor

captncraig commented Aug 25, 2016

In general, it looks pretty good. I'd just like to reduce some of the duplicated code if possible.

@gbrayut

This comment has been minimized.

Show comment
Hide comment
@gbrayut

gbrayut Aug 25, 2016

Contributor

After speaking with Craig the new commits remove the client code from collect (just use collect.DefaultClient to override) and update the scollector/docs.go file. We decided against a util function, since bosun already requires a custom transport and we may need to add additional custom headers in scollector/tsdbrelay the future.

I'll test on a few systems then merge this to master.

Contributor

gbrayut commented Aug 25, 2016

After speaking with Craig the new commits remove the client code from collect (just use collect.DefaultClient to override) and update the scollector/docs.go file. We decided against a util function, since bosun already requires a custom transport and we may need to add additional custom headers in scollector/tsdbrelay the future.

I'll test on a few systems then merge this to master.

@gbrayut gbrayut merged commit c1d4c83 into master Aug 25, 2016

2 checks passed

bosun All checks Passed!
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@gbrayut gbrayut deleted the scollector branch Aug 25, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment