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

Datasource: Add custom headers on alerting queries #19508

Merged
merged 5 commits into from Oct 11, 2019

Conversation

weeco
Copy link
Contributor

@weeco weeco commented Sep 30, 2019

Reference issue #15381

Which issue(s) this PR fixes:

Fixes #15381

I am not sure whether this pull request fixes the problem for all datasources, but it does fix the Problem for the Prometheus datasource. Thus I have omitted the keyword which automatically closes the referenced issue.

Special notes for your reviewer:

This PR does not add any further tests, as I wasn't sure how I can implement the tests for the custom headers. Additionally this breaks the existing datasource_cache.test as it is testing the TLS configuration.

What's the best way to handle this? Maintainers' changes obviously welcome.

Reference issue grafana#15381

Signed-off-by: Martin Schneppenheim <martin.schneppenheim@rewe-digital.com>
@CLAassistant
Copy link

CLAassistant commented Sep 30, 2019

CLA assistant check
All committers have signed the CLA.

@weeco weeco changed the title Add custom headers on alerting queries Alerting/Datasource: Add custom headers on alerting queries Sep 30, 2019
@marefr
Copy link
Member

marefr commented Sep 30, 2019

@weeco thanks.

First of all you have some linting errors:

# github.com/grafana/grafana/pkg/models [github.com/grafana/grafana/pkg/models.test]
pkg/models/datasource_cache_test.go:34:9: t1.TLSClientConfig undefined (type http.RoundTripper has no field or method TLSClientConfig)
pkg/models/datasource_cache_test.go:37:13: t1.TLSClientConfig undefined (type http.RoundTripper has no field or method TLSClientConfig)
pkg/models/datasource_cache_test.go:40:9: t1.TLSClientConfig undefined (type http.RoundTripper has no field or method TLSClientConfig)
pkg/models/datasource_cache_test.go:65:9: t1.TLSClientConfig undefined (type http.RoundTripper has no field or method TLSClientConfig)
pkg/models/datasource_cache_test.go:68:13: t1.TLSClientConfig undefined (type http.RoundTripper has no field or method TLSClientConfig)
pkg/models/datasource_cache_test.go:71:9: t1.TLSClientConfig undefined (type http.RoundTripper has no field or method TLSClientConfig)
pkg/models/datasource_cache_test.go:82:9: t2.TLSClientConfig undefined (type http.RoundTripper has no field or method TLSClientConfig)
pkg/models/datasource_cache_test.go:113:9: tr.TLSClientConfig undefined (type http.RoundTripper has no field or method TLSClientConfig)
pkg/models/datasource_cache_test.go:116:13: tr.TLSClientConfig undefined (type http.RoundTripper has no field or method TLSClientConfig)
pkg/models/datasource_cache_test.go:142:9: tr.TLSClientConfig undefined (type http.RoundTripper has no field or method TLSClientConfig)
pkg/models/datasource_cache_test.go:142:9: too many errors
make: *** [Makefile:111: go-vet] Error 2
Exited with code 2

Then you have some nil-pointer errors: https://circleci.com/gh/grafana/grafana/159848?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-checks-link

@weeco
Copy link
Contributor Author

weeco commented Oct 1, 2019

@marefr Exactly I meant these linting errors and I am not sure how you want me to handle these. Since I return the http.RoundTripper in the GetHttpTransport method, the TLSClientConfig can not be accessed anymore.

What do you recommend here? I'll take a look at the failing test in the meantime.

@marefr
Copy link
Member

marefr commented Oct 1, 2019

Oh right. I think you have to return something else than http.RoundTripper, like a new struct. Maybe something along the lines of this?

type cachedTransport struct {
	updated time.Time

	*dataSourceTransport
}

type dataSourceTransport struct {
	roundTripper    http.RoundTripper
	transport *http.Transport
}

...

func (ds *DataSource) GetHttpTransport() (*dataSourceTransport, error) {

...

// RoundTrip implements http.RoundTripper.
func (dst *dataSourceTransport) RoundTrip(req *http.Request) (*http.Response, error) {
	return dst.roundTripper.RoundTrip(req)
}

@weeco
Copy link
Contributor Author

weeco commented Oct 1, 2019

@marefr I think you can give this another look now.

@marefr marefr self-requested a review October 7, 2019 22:15
@rjmasikome
Copy link

Any update?

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.

Sorry it took some time for me to get back to this. Great work 👍 Some test-related changes left before we can merge to make sure that we have some additional test-coverage for the future in case we change/put in additional logic around round trippers. See comment for detail.

Have verified that it's working as expected using Prometheus:
image
image

Have verified that it's working as expected using Elasticsearch:
image
image

pkg/models/datasource_cache_test.go Show resolved Hide resolved
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.

Perfect 👍 Great work!

@marefr marefr merged commit 8cd93f0 into grafana:master Oct 11, 2019
Backend Platform Backlog automation moved this from Inbox to Done Oct 11, 2019
@marefr marefr changed the title Alerting/Datasource: Add custom headers on alerting queries Datasource: Add custom headers on alerting queries Oct 11, 2019
@marefr marefr added this to the 6.5 milestone Oct 11, 2019
@marefr
Copy link
Member

marefr commented Oct 11, 2019

@weeco Thank you for contributing to Grafana!

@ying-jeanne ying-jeanne added the pr/external This PR is from external contributor label Apr 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

Custom headers for http datasources are not used for alerting queries
5 participants