[exporter/bmchelix] Second PR of New component: BMC Helix Exporter #37350
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,27 @@ | ||
| # Use this changelog template to create an entry for release notes. | ||
|
|
||
| # One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix' | ||
| change_type: new_component | ||
|
|
||
| # The name of the component, or a single word describing the area of concern, (e.g. filelogreceiver) | ||
| component: bmchelixexporter | ||
|
|
||
| # A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`). | ||
| note: metrics implementation | ||
|
|
||
| # Mandatory: One or more tracking issues related to the change. You can use the PR number here if no issue exists. | ||
| issues: [36773] | ||
|
|
||
| # (Optional) One or more lines of additional information to render under the primary note. | ||
| # These lines will be padded with 2 spaces and then inserted directly into the document. | ||
| # Use pipe (|) for multiline entries. | ||
| subtext: | ||
|
|
||
| # If your change doesn't affect end users or the exported elements of any package, | ||
| # you should instead start your pull request title with [chore] or use the "Skip Changelog" label. | ||
| # Optional: The change log or logs in which this entry should be included. | ||
| # e.g. '[user]' or '[user, api]' | ||
| # Include 'user' if the change is relevant to end users. | ||
| # Include 'api' if there is a change to a library API. | ||
| # Default: '[user]' | ||
| change_logs: [user] |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,84 @@ | ||
| // Copyright The OpenTelemetry Authors | ||
| // SPDX-License-Identifier: Apache-2.0 | ||
|
|
||
| package bmchelixexporter // import "github.com/open-telemetry/opentelemetry-collector-contrib/exporter/bmchelixexporter" | ||
|
|
||
| import ( | ||
| "context" | ||
| "errors" | ||
| "os" | ||
|
|
||
| "go.opentelemetry.io/collector/component" | ||
| "go.opentelemetry.io/collector/exporter" | ||
| "go.opentelemetry.io/collector/pdata/pmetric" | ||
| "go.uber.org/zap" | ||
| ) | ||
|
|
||
| // BmcHelixExporter is responsible for exporting metrics to BMC Helix | ||
| type BmcHelixExporter struct { | ||
| config *Config | ||
| logger *zap.Logger | ||
| version string | ||
| telemetrySettings component.TelemetrySettings | ||
| metricsProducer *MetricsProducer | ||
| metricsClient *MetricsClient | ||
| } | ||
|
|
||
| // newBmcHelixExporter instantiates a new BMC Helix Exporter | ||
| func newBmcHelixExporter(config *Config, createSettings exporter.Settings) (*BmcHelixExporter, error) { | ||
| if config == nil { | ||
| return nil, errors.New("nil config") | ||
| } | ||
|
|
||
| return &BmcHelixExporter{ | ||
| config: config, | ||
| version: createSettings.BuildInfo.Version, | ||
| logger: createSettings.Logger, | ||
| telemetrySettings: createSettings.TelemetrySettings, | ||
| }, nil | ||
| } | ||
|
|
||
| // pushMetrics is invoked by the OpenTelemetry Collector to push metrics to BMC Helix | ||
| func (be *BmcHelixExporter) pushMetrics(ctx context.Context, md pmetric.Metrics) error { | ||
| be.logger.Info("Building BMC Helix payload") | ||
| helixMetrics, err := be.metricsProducer.ProduceHelixPayload(md) | ||
| if err != nil { | ||
| be.logger.Error("Failed to build BMC Helix payload", zap.Error(err)) | ||
| 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. |
||
| err = be.metricsClient.SendHelixPayload(ctx, helixMetrics) | ||
| if err != nil { | ||
| be.logger.Error("Failed to send BMC Helix payload", zap.Error(err)) | ||
| return err | ||
| } | ||
|
|
||
| return nil | ||
| } | ||
|
|
||
| // start is invoked during service start | ||
| func (be *BmcHelixExporter) start(ctx context.Context, host component.Host) error { | ||
| 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 There was a problem hiding this comment. That's a good catch. I recommend we don't use And if metrics don't have a There was a problem hiding this comment. Ok, |
||
| if err != nil { | ||
| be.logger.Error("Failed to get OS hostname", zap.Error(err)) | ||
| return err | ||
| } | ||
|
|
||
| // Initialize and store the MetricsProducer | ||
| be.metricsProducer = newMetricsProducer(osHostname, be.logger) | ||
|
|
||
| // Initialize and store the MetricsClient | ||
| metricsClient, err := newMetricsClient(ctx, be.config, host, be.telemetrySettings, be.logger) | ||
| if err != nil { | ||
| be.logger.Error("Failed to create MetricsClient", zap.Error(err)) | ||
| return err | ||
| } | ||
| be.metricsClient = metricsClient | ||
|
|
||
| be.logger.Info("Initialized BMC Helix Exporter") | ||
| return nil | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,28 @@ | ||
| // Copyright The OpenTelemetry Authors | ||
| // SPDX-License-Identifier: Apache-2.0 | ||
|
|
||
| package bmchelixexporter | ||
|
|
||
| import ( | ||
| "testing" | ||
|
|
||
| "github.com/stretchr/testify/assert" | ||
| "go.opentelemetry.io/collector/exporter/exportertest" | ||
| ) | ||
|
|
||
| func TestNewBmcHelixExporterWithNilConfig(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| exp, err := newBmcHelixExporter(nil, exportertest.NewNopSettings()) | ||
| assert.Nil(t, exp) | ||
| assert.Error(t, err) | ||
| } | ||
|
|
||
| func TestNewBmcHelixExporterWithDefaultConfig(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| cfg := createDefaultConfig().(*Config) | ||
| exp, err := newBmcHelixExporter(cfg, exportertest.NewNopSettings()) | ||
| assert.NotNil(t, exp) | ||
| assert.NoError(t, err) | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You want to use
configopaque.Stringavoid leaking the key within logs or other marshalling.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, good idea, thank you!