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

Aws prw exporter #2

Closed
wants to merge 26 commits into from
Closed

Aws prw exporter #2

wants to merge 26 commits into from

Conversation

amanbrar1999
Copy link

@amanbrar1999 amanbrar1999 commented Nov 13, 2020

Description:
This PR introduces a new exporter that leverages the existing Prometheus Remote Write Exporter in the core collector repo, and adds AWS Sigv4 authentication logic to it.

The idea here is to add an interceptor to the http client that is used to initialize the Prometheus Remote Write Exporter struct. The interceptor handles the Sigv4 logic, and the rest of the logic is identical to the core exporter.

The way this is designed, any changes in the core exporter's config will need to be duplicated here, however any other changes in the core exporter should be fine.

This PRs size is significantly bloated by the go summary file

Link to tracking Issue:
No direct issue for this exists but it is related to vendor specific code discussion: open-telemetry/oteps#141

Testing:
Unit tests have been added for the factory and config. Testing the authentication logic has been done manually by setting up a pipeline to communicate with AWS services.

Documentation:
A thorough README.md is included, as well as all example configuration in testdata/config.yaml

Copy link

@PettitWesley PettitWesley left a comment

Choose a reason for hiding this comment

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

2 new comments, but overall code lgtm

I think making the auth module generic so that it can be used to other AWS exporters could be a future PR/refactoring if you want.

Copy link

@rakyll rakyll left a comment

Choose a reason for hiding this comment

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

Left come comments, otherwise looks good to me.

prw "go.opentelemetry.io/collector/exporter/prometheusremotewriteexporter"
)

const (
Copy link

Choose a reason for hiding this comment

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

You can remove the grouping here when there is only one const:

const typeStr = "awsprometheusremotewrite" // The value of "type" key in configuration.

"go.uber.org/zap"
)

func Test_Type(t *testing.T) {
Copy link

Choose a reason for hiding this comment

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

TestType

_ is used often for different flavors of testing for the same. For example:

TestType_edcase1
TestType_edcase2

}

//Tests whether or not the default Exporter factory can instantiate a properly interfaced Exporter with default conditions
func Test_CreateDefaultConfig(t *testing.T) {
Copy link

Choose a reason for hiding this comment

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

TestCreateDefaultConfig

}

//Tests whether or not a correct Metrics Exporter from the default Config parameters
func Test_CreateMetricsExporter(t *testing.T) {
Copy link

Choose a reason for hiding this comment

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

TestCreateMetricsExporter

return cfg
}

func validateAuthConfig(params AuthSettings) bool {
Copy link

Choose a reason for hiding this comment

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

I'd rename this to isAuthSettingsValid.

@amanbrar1999
Copy link
Author

All comments have been resolved and PR has been merged upstream

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.

4 participants