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.

Sign up for GitHub

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

Jump to bottom

Datasource: Fix dataproxy timeout should always be applied for outgoing data source HTTP requests #34597

Merged
merged 4 commits into from May 25, 2021
Merged

Datasource: Fix dataproxy timeout should always be applied for outgoing data source HTTP requests #34597

merged 4 commits into from May 25, 2021

Conversation

dsotirakis
Copy link
Contributor

@dsotirakis dsotirakis commented May 24, 2021 •
edited by marefr

What this PR does / why we need it:

ResponseHeaderTimeout is now set for the http.Transport via grafana/grafana-plugin-sdk-go#358 which should guarantee that outgoing data source HTTP requests that do not directly use a http.Client, rather a http.Transport, will get the ResponseHeaderTimeout set to the configured dataproxy timeout. The Grafana data source proxy and the Prometheus data source (when queries comes from Grafana alerting) are two examples where the dataproxy timeout was not properly enforced, but this change should fix this.

In addition, a new data proxy setting named dialTimeout was introduced.

image

Which issue(s) this PR fixes:

Fixes #34177
Fixes #29457

@dsotirakis dsotirakis added this to the 8.0.0-beta3 milestone May 24, 2021
@dsotirakis dsotirakis requested a review from marefr May 24, 2021 14:19
@dsotirakis dsotirakis self-assigned this May 24, 2021
@dsotirakis dsotirakis requested a review from a team as a code owner May 24, 2021 14:19
@dsotirakis dsotirakis removed the request for review from a team May 24, 2021 14:19
@dsotirakis dsotirakis added this to In progress in Grafana Backend (DO NOT USE!) via automation May 24, 2021
@dsotirakis dsotirakis requested a review from wbrowne May 24, 2021 14:19
dsotirakis
dsotirakis commented May 24, 2021
View reviewed changes
go.mod Outdated
@@ -51,7 +51,7 @@ require (
github.com/gosimple/slug v1.9.0
github.com/grafana/grafana-aws-sdk v0.4.0
github.com/grafana/grafana-live-sdk v0.0.6
github.com/grafana/grafana-plugin-sdk-go v0.99.0
github.com/grafana/grafana-plugin-sdk-go v0.99.1-0.20210524132954-f5c0098028e0
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can create a tag here

Copy link
Member

Choose a reason for hiding this comment

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

Please go ahead tag a new version - v0.100.0 🎉 and update this accordingly. Instructions here

@dsotirakis dsotirakis changed the title HTTP Client: Add ResponseHeaderTimeout - split from DialContext t… HTTP Client: Make ResponseHeaderTimeout default timeout in http client May 24, 2021
marefr
marefr requested changes May 24, 2021
View reviewed changes
Copy link
Member

@marefr marefr 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, but left some suggestions/request of changes.

conf/defaults.ini Outdated Show resolved Hide resolved
conf/defaults.ini Outdated Show resolved Hide resolved
conf/sample.ini Outdated Show resolved Hide resolved
go.mod Outdated
@@ -51,7 +51,7 @@ require (
github.com/gosimple/slug v1.9.0
github.com/grafana/grafana-aws-sdk v0.4.0
github.com/grafana/grafana-live-sdk v0.0.6
github.com/grafana/grafana-plugin-sdk-go v0.99.0
github.com/grafana/grafana-plugin-sdk-go v0.99.1-0.20210524132954-f5c0098028e0
Copy link
Member

Choose a reason for hiding this comment

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

Please go ahead tag a new version - v0.100.0 🎉 and update this accordingly. Instructions here

pkg/models/datasource_cache.go Outdated Show resolved Hide resolved
@marefr
Copy link
Member

marefr commented May 24, 2021

Should we reference closing this issue as well in the PR description 29457?

@dsotirakis dsotirakis requested a review from marefr May 25, 2021 05:44
marefr
marefr reviewed May 25, 2021
View reviewed changes
Copy link
Member

@marefr marefr 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 👍 but need to tag a new version of the sdk and update go.mod - see comment for instructions

@dsotirakis
Copy link
Contributor Author

Looks good 👍 but need to tag a new version of the sdk and update go.mod - see comment for instructions

@marefr, yep, just waiting for people to test it so we won't need to tag again!

@dsotirakis
Copy link
Contributor Author

@marefr Should also be added to changelog and be backported once verified that it works, yes?

@marefr
Copy link
Member

marefr commented May 25, 2021

Yes

@dsotirakis dsotirakis added add to changelog old backport v8.0.x Mark PR for automatic backport to v8.0.x labels May 25, 2021
@dsotirakis dsotirakis requested a review from marefr May 25, 2021 07:49
marefr
marefr approved these changes May 25, 2021
View reviewed changes
Copy link
Member

@marefr marefr left a comment

Choose a reason for hiding this comment

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

LGTM

@dsotirakis dsotirakis merged commit 91657da into main May 25, 2021
Grafana Backend (DO NOT USE!) automation moved this from In progress to Done May 25, 2021
@dsotirakis dsotirakis deleted the split-response-header-timeout-and-dialer-context-timeout branch May 25, 2021 08:32
grafanabot pushed a commit that referenced this pull request May 25, 2021
@dsotirakis @grafanabot
HTTP Client: Make ResponseHeaderTimeout default timeout in http cli…
711ebe9 
…ent (#34597)

* HTTP Client: Add `ResponseHeaderTimeout` - split from `DialContext` timeout

* Fixes according to reviewer's comments

* Use grafana-plugin-sdk-go v0.100.0

(cherry picked from commit 91657da)
@grafanabot grafanabot mentioned this pull request May 25, 2021
Merged
dsotirakis added a commit that referenced this pull request May 25, 2021
@grafanabot @dsotirakis
[v8.0.x] HTTP Client: Make ResponseHeaderTimeout default timeout in…
3956e2b 
… http client (#34629)

* HTTP Client: Make `ResponseHeaderTimeout` default timeout in http client (#34597)

* HTTP Client: Add `ResponseHeaderTimeout` - split from `DialContext` timeout

* Fixes according to reviewer's comments

* Use grafana-plugin-sdk-go v0.100.0

(cherry picked from commit 91657da)

* Small change

Co-authored-by: Dimitris Sotirakis <dimitrios.sotirakis@grafana.com>
Co-authored-by: dsotirakis <sotirakis.dim@gmail.com>
@marefr marefr changed the title HTTP Client: Make ResponseHeaderTimeout default timeout in http client Datasource: Fix dataproxy timeout should always be applied for outgoing data source HTTP requests May 25, 2021
@marefr marefr mentioned this pull request May 25, 2021
Closed
@gabor gabor mentioned this pull request May 26, 2021
Closed
@zoltanbedi zoltanbedi mentioned this pull request Jul 14, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@marefr marefr marefr approved these changes

@wbrowne wbrowne Awaiting requested review from wbrowne

Assignees

@dsotirakis dsotirakis

Labels
add to changelog area/backend/api area/backend area/http-server old backport v8.0.x Mark PR for automatic backport to v8.0.x type/bug
Projects
No open projects
Grafana Backend (DO NOT USE!)
Done
Milestone
8.0.0-beta3
2 participants
@dsotirakis @marefr