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

[Prometheus Remote Write Exporter for Cortex] Add TLS Support and Default HTTP Client #255

Merged
merged 27 commits into from
Aug 26, 2020

Conversation

ercl
Copy link
Contributor

@ercl ercl commented Aug 21, 2020

This PR implements TLS support for the Go SDK Prometheus Remote Write Exporter for Cortex. It includes functionality for creating a default HTTP client for the exporter, unit tests, and an integration test for mutual TLS.

Copy link
Contributor

@jmacd jmacd left a comment

Choose a reason for hiding this comment

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

👍

exporters/metric/cortex/auth.go Show resolved Hide resolved
exporters/metric/cortex/auth.go Outdated Show resolved Hide resolved
exporters/metric/cortex/auth.go Outdated Show resolved Hide resolved
exporters/metric/cortex/auth.go Outdated Show resolved Hide resolved
Copy link
Contributor

@MrAlias MrAlias 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 in the overall approach. The previous review (sorry about the partial submit of that) contained some things that need to be addressed and this one has more nice-to-haves.

Great job.

exporters/metric/cortex/auth_test.go Outdated Show resolved Hide resolved
exporters/metric/cortex/auth_test.go Outdated Show resolved Hide resolved
exporters/metric/cortex/auth_test.go Outdated Show resolved Hide resolved
exporters/metric/cortex/auth_test.go Outdated Show resolved Hide resolved
@ercl
Copy link
Contributor Author

ercl commented Aug 26, 2020

Thanks for all the feedback! The recent commits should address everything in the reviews.

Copy link
Contributor

@MrAlias MrAlias left a comment

Choose a reason for hiding this comment

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

🚀

@MrAlias
Copy link
Contributor

MrAlias commented Aug 26, 2020

@ercl this looks ready to merge. If you can update with the master branch it should be good to go.

@ercl
Copy link
Contributor Author

ercl commented Aug 26, 2020

Merged! I also updated the HTTP Transport's DialContext timeout since the mutual tls test was occasionally taking too much time on GOARCH 386.

@MrAlias
Copy link
Contributor

MrAlias commented Aug 26, 2020

Merged! I also updated the HTTP Transport's DialContext timeout since the mutual tls test was occasionally taking too much time on GOARCH 386.

Hmm, looks like this is still out of sync. I'll be sure to not merge anything before this if you can update again.

@ercl
Copy link
Contributor Author

ercl commented Aug 26, 2020

Merged again! I think the reason why CircleCI tests fail is because the testing script sets the timeout at 30s, which the Mutual TLS test occasionally goes over.

@MrAlias MrAlias merged commit 1f7546c into open-telemetry:master Aug 26, 2020
plantfansam referenced this pull request in plantfansam/opentelemetry-go-contrib Mar 18, 2022
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.

None yet

3 participants