[chore] [exporter/alertmanagerexporter] Add exporter to components #30230
Conversation
Added a commit with the change, thank you. |
|
Hello @bryan-aguilar, the PR needs one more review/approval. We'd appreciate your help! |
There was a problem hiding this comment.
Hi @sokoide,
For functional changes to PRs it is very helpful to have a codeowner approval on the PR. It gives collector approvers and maintainers, who may not be familiar with the component, more confidence in adding the ready to merge label on the PR.
Would you mind also reviewing the PR and adding your approval, comment, or change request?
Also, it looks like there are some merge conflicts that need to be resolved @mcube8
cmd/otelcontribcol/exporters_test.go
Outdated
| @@ -21,6 +21,7 @@ import ( | |||
| "go.opentelemetry.io/collector/exporter/otlpexporter" | |||
| "go.opentelemetry.io/collector/exporter/otlphttpexporter" | |||
|
|
|||
| alertmanagerexporter "github.com/open-telemetry/opentelemetry-collector-contrib/exporter/alertmanagerexporter" | |||
There was a problem hiding this comment.
Is there a reason that this requires a named import?
cmd/otelcontribcol/exporters_test.go
Outdated
| { | ||
| exporter: "alertmanager", | ||
| getConfigFn: func() component.Config { | ||
| cfg := expFactories["alertmanager"].CreateDefaultConfig().(*alertmanagerexporter.Config) | ||
| cfg.HTTPClientSettings = confighttp.HTTPClientSettings{ | ||
| Endpoint: "http://" + endpoint, | ||
| } | ||
| cfg.GeneratorURL = "opentelemetry-collector" | ||
| cfg.DefaultSeverity = "info" | ||
| // disable queue/retry to validate passing the test data synchronously | ||
| cfg.QueueSettings.Enabled = false | ||
| cfg.RetrySettings.Enabled = false | ||
| return cfg | ||
| }, | ||
| expectConsumeErr: true, | ||
| }, |
There was a problem hiding this comment.
This lifecycle test should be added to your metadata.yaml file instead of explicitly in the builder config. mdatagen will then automatically generate the configuration in the component. See the schema doc for reference.
I'm going to mark this as a requirement to merge the PR so that we do not create more work for ourselves in the future. @atoulme is spearheading a lot of work to deprecate this test file by removing lifecycle tests to components.
There was a problem hiding this comment.
Looks like this will be handled in https://github.com/open-telemetry/opentelemetry-collector-contrib/pull/30389/files
There was a problem hiding this comment.
#30389 is merged. This bit of code can be removed once you rebase from main.
There was a problem hiding this comment.
@bryan-aguilar, thank you for the review and merge. The feature itself has been merged by #27836 and #28906. I'm a co-owner of the feature, but I don't have a permission to add a tag or approve. If there is anything I can do, please suggest.
@mcube8 and I will take a look at #30389 and rebase. Thank you!
There was a problem hiding this comment.
@sokoide do you have the requisite permission to review PRs in the repository? I understand they won't be "green check mark" but even "grey check mark" helps. If you don't have permission for "grey check mark" review, then that may be an issue we need to raise.
There was a problem hiding this comment.
@bryan-aguilar Thanks for your review. I updated the test config in metadata.yml under alertmanager exporter component. When removing above snippet from exporters_test, the unittest check failed.
Kindly suggest if any further changes are required, thanks!
…pen-telemetry#30230) Add Alertmanager Exporter to Components Third PR - Update buildconfig and components **Link to tracking Issue:** [open-telemetry#23659](open-telemetry#23569) **Testing:** Unit tests for exporter **Documentation:** Readme and Sample Configs to use Alertmanager exporter --------- Co-authored-by: Alex Boten <aboten@lightstep.com>
…pen-telemetry#30230) Add Alertmanager Exporter to Components Third PR - Update buildconfig and components **Link to tracking Issue:** [open-telemetry#23659](open-telemetry#23569) **Testing:** Unit tests for exporter **Documentation:** Readme and Sample Configs to use Alertmanager exporter --------- Co-authored-by: Alex Boten <aboten@lightstep.com>
Description: Add Alertmanager Exporter to Components
Third PR - Update buildconfig and components
Link to tracking Issue: #23659
Testing: Unit tests for exporter
Documentation: Readme and Sample Configs to use Alertmanager exporter