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

Add aws prometheus remote write exporter #1497

Merged
merged 29 commits into from
Nov 17, 2020

Conversation

amanbrar1999
Copy link
Contributor

@amanbrar1999 amanbrar1999 commented Nov 5, 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

-- cc @alolita

@amanbrar1999 amanbrar1999 marked this pull request as draft November 5, 2020 05:25
Aman Brar added 2 commits November 5, 2020 12:14
Merge branch 'add-aws-cortex-exporter' of github.com:open-o11y/opentelemetry-collector-contrib into add-aws-cortex-exporter
@amanbrar1999 amanbrar1999 marked this pull request as ready for review November 6, 2020 20:05
Copy link
Member

@kbrockhoff kbrockhoff left a comment

Choose a reason for hiding this comment

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

LGTM

@bogdandrutu
Copy link
Member

@amanbrar1999 any update on this?

@amanbrar1999
Copy link
Contributor Author

@bogdandrutu We are reviewing the design of this component internally, I will convert to draft for now and reopen when ready

@amanbrar1999 amanbrar1999 marked this pull request as draft November 13, 2020 20:36
@bogdandrutu
Copy link
Member

@amanbrar1999 I am fine if you want to have this discussion here, now that I have context on the change, up to you

@amanbrar1999 amanbrar1999 marked this pull request as ready for review November 14, 2020 01:29
@amanbrar1999
Copy link
Contributor Author

@bogdandrutu I have embedded the factories, but i'm not sure if it has become as simple as you expected. The closure function can not be added in CreateDefaultConfig() because the user specified configuration is not yet present at that stage, hence I needed to also override CreateMetricsExporter(). I also had to override Type() because otherwise it took the type of the embedded exporter.

CreateMetricsExporter() cannot be simplified by calling CreateMetricsExporter() of the embedded factory because we cannot pass in a superset of the embedded factory's config, hence this function remains similar to how it was before

@bogdandrutu
Copy link
Member

@bogdandrutu I have embedded the factories, but i'm not sure if it has become as simple as you expected. The closure function can not be added in CreateDefaultConfig() because the user specified configuration is not yet present at that stage, hence I needed to also override CreateMetricsExporter().

Not sure, in the closure you save a pointer to the config, and the closure runs after the config parsing, am I missing something?

I also had to override Type() because otherwise it took the type of the embedded exporter.

True

CreateMetricsExporter() cannot be simplified by calling CreateMetricsExporter() of the embedded factory because we cannot pass in a superset of the embedded factory's config, hence this function remains similar to how it was before

Why? You can pass:

oCfg := cfg.(*Config)
...
return af.ExporterFactory(&oCfg.Config, ....)

@amanbrar1999
Copy link
Contributor Author

@bogdandrutu It all makes sense and works now, overriding Type() as well as the last point you made was all that was required to add to your code snippet for it to work since we needed to ensure only the embedded config was passed into the embedded CreateMetricsExporter() function. No more code is being copied over from the core exporter into this factory now with these changes.

@bogdandrutu bogdandrutu merged commit 3761763 into open-telemetry:master Nov 17, 2020
dyladan referenced this pull request in dynatrace-oss-contrib/opentelemetry-collector-contrib Jan 29, 2021
* Move some content from correctness_test.go to utils.go

This change makes these functions/types available from the metrics
package, where they will be needed to address issue #652.

* Add comments for exported types and fcns

* Address PR comments

* Fix lint
ljmsc referenced this pull request in ljmsc/opentelemetry-collector-contrib Feb 21, 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.

6 participants