[exporter/bmchelix] Second PR of New component: BMC Helix Exporter #37350

NassimBtk · 2025-01-20T19:16:32Z

Implemented core functionality in exporter.go to handle metric transformation and payload dispatch to BMC Helix.
Added metrics_client.go with HTTP client configuration.
Added metrics_producer.go to produce BMC Helix-compatible payloads.
Added unit tests.

Description

This pull request introduces the BMC Helix Exporter, which is responsible for exporting metrics to BMC Helix Operations Management. The most important changes include the implementation of the exporter, configuration adjustments, and the addition of unit tests.

Implementation of BMC Helix Exporter:

exporter/bmchelixexporter/exporter.go: Introduced BmcHelixExporter class with methods to start the exporter and push metrics to BMC Helix.
Developed metrics_producer.go to build the BMC Helix Operations Management expected metric payloads.
exporter/bmchelixexporter/metrics_client.go: Added MetricsClient class responsible for sending metrics payloads to BMC Helix.

Configuration Adjustments:

exporter/bmchelixexporter/config.go: Replaced Endpoint and Timeout fields with confighttp.ClientConfig to handle HTTP client configuration.
exporter/bmchelixexporter/factory.go: Updated createDefaultConfig to use confighttp.NewDefaultClientConfig and adjusted the createMetricsExporter function to initialize the exporter properly.

Unit Tests:

exporter/bmchelixexporter/config_test.go: Modified tests to accommodate changes in configuration structure and added helper function createDefaultClientConfig.
exporter/bmchelixexporter/exporter_test.go: Added unit tests for newBmcHelixExporter function to ensure proper initialization.
exporter/bmchelixexporter/metrics_producer.go: Added unit tests.
exporter/bmchelixexporter/metrics_client.go: Added unit tests.

Changelog Entry:

.chloggen/bmchelixexporter-metrics-implementation.yaml: Added a new changelog entry for the BMC Helix Exporter metrics implementation.

Link to tracking issue

Fixes #36773

* Implemented core functionality in `exporter.go` to handle metric transformation and payload dispatch to BMC Helix. * Added `metrics_client.go` with HTTP client configuration. * Added `metrics_producer.go` to produce BMC Helix-compatible payloads. * Added unit tests.

bertysentry · 2025-01-21T22:33:00Z

exporter/bmchelixexporter/exporter.go

+		return err
+	}
+
+	be.logger.Info("Sending BMC Helix payload")


Is it common practice to log all this? I would have expected such messages ("Building", "Sending", etc.) to be in debug level, rather than Info. (but I'm not familiar with the logging in other components!)

I think we can remove these two logs. I don't see what value they add, and after reviewing a few exporters, I didn't notice a similar practice to what I implemented here...

This is going to be a noisey log and potentially provide little value, can you remove this?

Yes, I will remove it.

bertysentry · 2025-01-21T22:39:57Z

exporter/bmchelixexporter/metrics_producer.go

+	}
+
+	// Check if the entityTypeId is set
+	if labels["entityTypeId"] == "" {


Too bad we can't figure out there is no entityTypeId attribute before processing all data points 🤷‍♂️

NassimBtk · 2025-01-22T17:31:23Z

Conflict to be resolved...

MovieStoreGuy · 2025-01-24T02:17:39Z

exporter/bmchelixexporter/config.go

-	Timeout     time.Duration             `mapstructure:"timeout"`
-	RetryConfig configretry.BackOffConfig `mapstructure:"retry_on_failure"`
+	confighttp.ClientConfig `mapstructure:",squash"`
+	APIKey                  string                    `mapstructure:"api_key"`


You want to use configopaque.String avoid leaking the key within logs or other marshalling.

Yes, good idea, thank you!

MovieStoreGuy · 2025-01-24T02:19:23Z

exporter/bmchelixexporter/exporter.go

+	be.logger.Info("Starting BMC Helix Exporter")
+
+	// Get the hostname reported by the kernel
+	osHostname, err := os.Hostname()


Depending on this starts up, it could fetch the container hostname which is just a hash, is that going to be an issue?

Very good question! This shouldn't cause issues for the user, as they should be able to configure the hostname of their container or pod. If that's not feasible, indeed, the hash will change with each new deployment. In such cases, I would recommend using OTTL to override the host.name on either the resource or the metric itself, though this approach could become cumbersome in scenarios where the collector processes a large volume of metrics. Alternatively, we could introduce an entity_hostname configuration option to override the OS hostname. Currently, the code defaults to the OS hostname only if it cannot find a host.name attribute on the datapoint or the associated resource. What do you think, @bertysentry?

That's a good catch. I recommend we don't use os.Hostname() as the default entity hostname in BMC Helix. Instead, if the user wants to use the local host details, we will recommend to simply use the resourcedetectionprocessor in their otelcol config.

And if metrics don't have a host.name (at the end of the processors chain), then such metrics won't be exported to BMC Helix, and it's okay. Maybe we should log the number of metrics that are dropped so the end user knows what is going on.

Ok, os.Hostname() will not be used in that case.

feature/issue-36773-new-component-bmc-helix-exporter-implementation

* Using configopaque.String for APIKey * os.Hostname is not used as last resort hostname * Removed noisy log info.

feature/issue-36773-new-component-bmc-helix-exporter-implementation

…exporter-implementation

feature/issue-36773-new-component-bmc-helix-exporter-implementation

MovieStoreGuy

Sorry to be a pain, but if we can shuffle some of the definitions of types into internal packages to reduce the API exposure would be awesome :D

MovieStoreGuy · 2025-02-03T05:51:11Z

exporter/bmchelixexporter/exporter.go

+)
+
+// BmcHelixExporter is responsible for exporting metrics to BMC Helix
+type BmcHelixExporter struct {


I would suggest not making this package in the root package, and by go naming convention it should be BMCHelixExporter (all acronyms get capitalised since they are new words).

Conversely, the name is also a double up on the package, perhaps having it just simply as producer would be enough to avoid any naming collisions with packages, and adhering to go naming conventions.

Thanks for the feedback! That makes sense. I'll rename BmcHelixExporter to metricsExporter since the package is already named bmchelixexporter, and metricsExporter should better reflect its purpose within the package.

MovieStoreGuy · 2025-02-03T05:52:38Z

exporter/bmchelixexporter/metrics_client.go

+// MetricsClient is responsible for sending the metrics payload to BMC Helix Operations Management
+type MetricsClient struct {


Just being mindful of what is being exported as part of the package, I think it just makes sense to rename as metricsClient and it should be fine here.

Thanks! I will rename the struct name to metricsClient.

MovieStoreGuy · 2025-02-03T05:55:59Z

exporter/bmchelixexporter/metrics_producer.go

Can I suggest that gets moved into internal/bmchelix (or anything that makes sense) to avoid exposing unneeded types in the root package?

Yes, that makes sense. Thank you!

feature/issue-36773-new-component-bmc-helix-exporter-implementation

* Moved MetricsProducer and MetricsClient to internal/ package to control API exposure * metricsExporter is now unexported

feature/issue-36773-new-component-bmc-helix-exporter-implementation

bertysentry

LGTM! Nice work Nassim!

* main: (392 commits) fix(deps): update module golang.org/x/text to v0.22.0 (open-telemetry#37686) [exporter/bmchelix] Second PR of New component: BMC Helix Exporter (open-telemetry#37350) chore(deps): update otel/opentelemetry-collector-contrib docker tag to v0.119.0 (open-telemetry#37688) [chore] fix codeowners allowlist (open-telemetry#37684) chore(deps): update otel/opentelemetry-collector docker tag to v0.119.0 (open-telemetry#37687) Update jpkroehling's affiliation (open-telemetry#37683) fix(deps): update module github.com/clickhouse/clickhouse-go/v2 to v2.30.3 (open-telemetry#37655) fix(deps): update all opentelemetry collector contrib packages to v0.119.0 (open-telemetry#37666) fix(deps): update module github.com/elastic/go-docappender/v2 to v2.4.0 (open-telemetry#37667) fix(deps): update all golang.org/x packages (open-telemetry#37680) [exporter/prometheusremotewrite] Fix WAL deadlock (open-telemetry#37630) fix(deps): update opentelemetry-go monorepo (open-telemetry#37673) fix(deps): update module github.com/shirou/gopsutil/v4 to v4.25.1 (open-telemetry#37671) fix(deps): update module github.com/spf13/pflag to v1.0.6 (open-telemetry#37658) fix(deps): update all github.com/aws packages (open-telemetry#37661) [chore] Prepare release 0.119.0 (open-telemetry#37660) make update-otel OTEL_VERSION=v0.119.0 OTEL_STABLE_VERSION=v1.25.0 (open-telemetry#37656) add documentation and warning log for deprecating AccessTokenPassthrough (open-telemetry#37575) chore: add myself, echlebek, as a codeowner (open-telemetry#37650) [processor/transform] Add support for flat configuration style (open-telemetry#37444) ...

NassimBtk requested a review from a team as a code owner January 20, 2025 19:16

NassimBtk requested a review from MovieStoreGuy January 20, 2025 19:16

github-actions bot assigned djaglowski Jan 20, 2025

github-actions bot added the exporter/bmchelix label Jan 20, 2025

github-actions bot requested a review from bertysentry January 20, 2025 19:16

bertysentry approved these changes Jan 21, 2025

View reviewed changes

MovieStoreGuy reviewed Jan 24, 2025

View reviewed changes

NassimBtk added 3 commits January 24, 2025 21:25

Merge remote-tracking branch 'origin/main' into

9e05545

feature/issue-36773-new-component-bmc-helix-exporter-implementation

[exporter/bmchelix] Second PR of New component: BMC Helix Exporter

6890faa

* Using configopaque.String for APIKey * os.Hostname is not used as last resort hostname * Removed noisy log info.

Merge remote-tracking branch 'origin/main' into

77d89bd

feature/issue-36773-new-component-bmc-helix-exporter-implementation

NassimBtk requested a review from MovieStoreGuy January 24, 2025 22:54

NassimBtk added 2 commits January 25, 2025 07:30

Merge branch 'main' into feature/issue-36773-new-component-bmc-helix-…

c198a42

…exporter-implementation

Merge remote-tracking branch 'origin/main' into

feb6978

feature/issue-36773-new-component-bmc-helix-exporter-implementation

MovieStoreGuy reviewed Feb 3, 2025

View reviewed changes

NassimBtk added 3 commits February 3, 2025 11:00

Merge remote-tracking branch 'origin/main' into

a745151

feature/issue-36773-new-component-bmc-helix-exporter-implementation

[exporter/bmchelix] Second PR of New component: BMC Helix Exporter

9a9ed15

* Moved MetricsProducer and MetricsClient to internal/ package to control API exposure * metricsExporter is now unexported

Merge remote-tracking branch 'origin/HEAD' into

17888b8

feature/issue-36773-new-component-bmc-helix-exporter-implementation

bertysentry approved these changes Feb 4, 2025

View reviewed changes

MovieStoreGuy approved these changes Feb 4, 2025

View reviewed changes

MovieStoreGuy merged commit 9b515fb into open-telemetry:main Feb 4, 2025
163 checks passed

github-actions bot added this to the next release milestone Feb 4, 2025

bertysentry deleted the feature/issue-36773-new-component-bmc-helix-exporter-implementation branch February 4, 2025 23:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[exporter/bmchelix] Second PR of New component: BMC Helix Exporter #37350

[exporter/bmchelix] Second PR of New component: BMC Helix Exporter #37350

NassimBtk commented Jan 20, 2025 •

edited

Loading

bertysentry Jan 21, 2025

NassimBtk Jan 22, 2025

MovieStoreGuy Jan 24, 2025

NassimBtk Jan 24, 2025

bertysentry Jan 21, 2025

NassimBtk commented Jan 22, 2025

MovieStoreGuy Jan 24, 2025

NassimBtk Jan 24, 2025

MovieStoreGuy Jan 24, 2025

NassimBtk Jan 24, 2025 •

edited

Loading

bertysentry Jan 24, 2025

NassimBtk Jan 24, 2025

MovieStoreGuy left a comment

MovieStoreGuy Feb 3, 2025

NassimBtk Feb 3, 2025

MovieStoreGuy Feb 3, 2025

NassimBtk Feb 3, 2025 •

edited

Loading

MovieStoreGuy Feb 3, 2025

NassimBtk Feb 3, 2025

bertysentry left a comment

		// MetricsClient is responsible for sending the metrics payload to BMC Helix Operations Management
		type MetricsClient struct {

[exporter/bmchelix] Second PR of New component: BMC Helix Exporter #37350

[exporter/bmchelix] Second PR of New component: BMC Helix Exporter #37350

Conversation

NassimBtk commented Jan 20, 2025 • edited Loading

Description

Implementation of BMC Helix Exporter:

Configuration Adjustments:

Unit Tests:

Changelog Entry:

Link to tracking issue

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NassimBtk commented Jan 22, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NassimBtk Jan 24, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MovieStoreGuy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NassimBtk Feb 3, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bertysentry left a comment

Choose a reason for hiding this comment

NassimBtk commented Jan 20, 2025 •

edited

Loading

NassimBtk Jan 24, 2025 •

edited

Loading

NassimBtk Feb 3, 2025 •

edited

Loading