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

pkg/canary: use default HTTP client when reading from Loki #1131

Merged
merged 1 commit into from
Oct 9, 2019

Conversation

rfratto
Copy link
Member

@rfratto rfratto commented Oct 8, 2019

The documentation for net/http.Client says the following:

A Client is an HTTP client. Its zero value (DefaultClient) is a usable client that uses DefaultTransport.

The Client's Transport typically has internal state (cached TCP connections), so Clients should be reused instead of created as needed. Clients are safe for concurrent use by multiple goroutines.

A Client is higher-level than a RoundTripper (such as Transport) and additionally handles HTTP details such as cookies and redirects.

Canary was creating a new http.Client every time a query request was made to Loki, causing a new TCP connection to be established each time. If Loki has an outage, it's possible to get into a situation where all websocket requests are failing and for a high volume of requests to go through the normal query route. This can possibly lead to networking issues like socket starvation.

The documentation[1] for net/http.Client says the following:

  A Client is an HTTP client. Its zero value (DefaultClient) is a usable
  client that uses DefaultTransport.

  The Client's Transport typically has internal state (cached TCP
  connections), so Clients should be reused instead of created as needed.
  Clients are safe for concurrent use by multiple goroutines.

  A Client is higher-level than a RoundTripper (such as Transport) and
  additionally handles HTTP details such as cookies and redirects.

Canary was creating a new http.Client every time a query request was
made to Loki, causing a new TCP connection to be established each time.
If Loki has an outage, it's possible to get into a situation where all
websocket requests are failing and for a high volume of requests to go
through the normal query route. This can possibly lead to networking
issues like socket starvation.

[1]: https://golang.org/pkg/net/http/#Client
@rfratto rfratto added component/agent type/enhancement Something existing could be improved labels Oct 8, 2019
@rfratto rfratto requested a review from slim-bean October 8, 2019 14:17
Copy link
Collaborator

@slim-bean slim-bean left a comment

Choose a reason for hiding this comment

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

LGTM!

@slim-bean slim-bean merged commit f1c1097 into grafana:master Oct 9, 2019
@rfratto rfratto deleted the canary-use-default-http-client branch November 19, 2019 14:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/agent type/enhancement Something existing could be improved
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants