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

[exporter/bmchelix] New component: BMC Helix Exporter #36964

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

NassimBtk
Copy link
Contributor

@NassimBtk NassimBtk commented Dec 26, 2024

Description

This pull request introduces a new component for exporting metrics to BMC Helix. The changes include adding the new component to various configuration files, creating necessary documentation, and implementing the component's configuration and factory logic.

Key changes include:

New Component Addition:
  • Added a new changelog entry for the BMC Helix exporter in .chloggen/bmchelixexporter-new-component.yaml.
  • Updated .github/CODEOWNERS to include the new BMC Helix exporter.
Documentation:
  • Created README.md for the BMC Helix exporter with detailed setup instructions and examples.
Configuration and Factory Implementation:
  • Implemented configuration struct and validation logic in config.go.
  • Created tests for the configuration in config_test.go.
  • Added factory methods for creating the exporter in factory.go.
  • Created tests for the factory methods in factory_test.go.
Miscellaneous:
  • Included the common Makefile in exporter/bmchelixexporter/Makefile.
  • Added package documentation in doc.go.

Link to tracking issue

Fixes #36773

* Go init
* Added README.md and code skeleton.
@NassimBtk NassimBtk requested a review from a team as a code owner December 26, 2024 13:23
* Updated go version to 1.22.0 in the go.mod file.
* Fixed dependency versions in go.mod
if c.ApiKey == "" {
return errors.New("api key is required")
}
if c.Timeout < 0 {
Copy link
Contributor

Choose a reason for hiding this comment

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

If timeout must be a positive integer, then I think it should be if c.Timeout <= 0

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch!

* Fixed timeout positive integer check.
@songy23 songy23 added ready to merge Code review completed; ready to merge by maintainers Accepted Component New component has been sponsored labels Dec 27, 2024
Copy link
Contributor

@bertysentry bertysentry left a comment

Choose a reason for hiding this comment

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

Review TBC

exporter/bmchelixexporter/README.md Outdated Show resolved Hide resolved
exporter/bmchelixexporter/README.md Outdated Show resolved Hide resolved
exporter/bmchelixexporter/README.md Outdated Show resolved Hide resolved
exporter/bmchelixexporter/README.md Outdated Show resolved Hide resolved
- `entityTypeId`: Type identifier for the entity.
- `instanceName`: Display name of the entity.

> **Note:** If `entityName` or `entityTypeId` is missing, the metric will not be populated.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
> **Note:** If `entityName` or `entityTypeId` is missing, the metric will not be populated.
> **Note:** If `entityName` or `entityTypeId` is missing, the metric will not be exported.
To ensure the necessary attributes are not present, it is recommended to leverage the [transform processor](https://github.com/open-telemetry/opentelemetry-collector-contrib/tree/main/processor/transformprocessor) with [OTTL](https://github.com/open-telemetry/opentelemetry-collector-contrib/tree/main/pkg/ottl), and include it in the configuration of the telemetry pipeline.
The minimal pipeline most often looks like: `OTEL metrics --> (batch/memory limit) --> transform processor --> bmchelix exporter`.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

are present, ...

- `entityTypeId`: Type identifier for the entity.
- `instanceName`: Display name of the entity.

> **Note:** If `entityName` or `entityTypeId` is missing, the metric will not be populated.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

are present, ...

exporter/bmchelixexporter/README.md Outdated Show resolved Hide resolved
* Apply suggestions from code review

Co-authored-by: Bertrand Martin <[email protected]>
enabled: true
```

### Resource to Telemetry Conversion
Copy link
Contributor

Choose a reason for hiding this comment

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

This option (resource_to_telemetry_conversion) should probably be removed as it's difficult to understand and is pretty useless in BMC Helix use case.

Copy link
Contributor

@fatsheep9146 fatsheep9146 left a comment

Choose a reason for hiding this comment

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

The go module should be updated

go: updates to go.mod needed; to update it:
	go mod tidy
/home/runner/work/opentelemetry-collector-contrib/opentelemetry-collector-contrib/.tools/govulncheck ./...
govulncheck: loading packages: err: exit status 1: stderr: go: updates to go.mod needed; to update it:
	go mod tidy

@NassimBtk
Copy link
Contributor Author

Thanks a lot @fatsheep9146, I am updating the component..., I just need to adjust the configuration a bit to remove the resource_to_telemetry_conversion config option as suggested by @bertysentry.

* Updated go module
* Removed the resource_to_telemetry_conversion option
* Update: go.opentelemetry.io/collector/confmap v1.23.0
* Fixed ApiKey var naming (APIKey)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Accepted Component New component has been sponsored ready to merge Code review completed; ready to merge by maintainers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

New component: BMC Helix Exporter
4 participants