[exporter/bmchelix] Second PR of New component: BMC Helix Exporter #37350
Conversation
* 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.
| return err | ||
| } | ||
|
|
||
| be.logger.Info("Sending BMC Helix payload") |
There was a problem hiding this comment.
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!)
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
This is going to be a noisey log and potentially provide little value, can you remove this?
There was a problem hiding this comment.
Yes, I will remove it.
| } | ||
|
|
||
| // Check if the entityTypeId is set | ||
| if labels["entityTypeId"] == "" { |
There was a problem hiding this comment.
Too bad we can't figure out there is no entityTypeId attribute before processing all data points 🤷♂️
|
Conflict to be resolved... |
exporter/bmchelixexporter/config.go
Outdated
| Timeout time.Duration `mapstructure:"timeout"` | ||
| RetryConfig configretry.BackOffConfig `mapstructure:"retry_on_failure"` | ||
| confighttp.ClientConfig `mapstructure:",squash"` | ||
| APIKey string `mapstructure:"api_key"` |
There was a problem hiding this comment.
You want to use configopaque.String avoid leaking the key within logs or other marshalling.
There was a problem hiding this comment.
Yes, good idea, thank you!
| be.logger.Info("Starting BMC Helix Exporter") | ||
|
|
||
| // Get the hostname reported by the kernel | ||
| osHostname, err := os.Hostname() |
There was a problem hiding this comment.
Depending on this starts up, it could fetch the container hostname which is just a hash, is that going to be an issue?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
exporter.goto handle metric transformation and payload dispatch to BMC Helix.metrics_client.gowith HTTP client configuration.metrics_producer.goto produce BMC Helix-compatible payloads.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: IntroducedBmcHelixExporterclass with methods to start the exporter and push metrics to BMC Helix.metrics_producer.goto build the BMC Helix Operations Management expected metric payloads.exporter/bmchelixexporter/metrics_client.go: AddedMetricsClientclass responsible for sending metrics payloads to BMC Helix.Configuration Adjustments:
exporter/bmchelixexporter/config.go: ReplacedEndpointandTimeoutfields withconfighttp.ClientConfigto handle HTTP client configuration.exporter/bmchelixexporter/factory.go: UpdatedcreateDefaultConfigto useconfighttp.NewDefaultClientConfigand adjusted thecreateMetricsExporterfunction to initialize the exporter properly.Unit Tests:
exporter/bmchelixexporter/config_test.go: Modified tests to accommodate changes in configuration structure and added helper functioncreateDefaultClientConfig.exporter/bmchelixexporter/exporter_test.go: Added unit tests fornewBmcHelixExporterfunction 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