-
Notifications
You must be signed in to change notification settings - Fork 524
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
Allow setting the metric batch exporter early #11545
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -34,12 +34,11 @@ import ( | |
// NewMetricExporter initializes a new MetricExporter | ||
// This export logic is heavily inspired from the OTLP input in apm-data. | ||
// https://github.com/elastic/apm-data/blob/main/input/otlp/metrics.go | ||
func NewMetricExporter(p modelpb.BatchProcessor, opts ...ConfigOption) *MetricExporter { | ||
func NewMetricExporter(opts ...ConfigOption) *MetricExporter { | ||
cfg := newConfig(opts...) | ||
|
||
return &MetricExporter{ | ||
processor: p, | ||
|
||
processor: cfg.processor, | ||
metricFilter: cfg.MetricFilter, | ||
temporalitySelector: cfg.TemporalitySelector, | ||
aggregationSelector: cfg.AggregationSelector, | ||
|
@@ -56,6 +55,11 @@ type MetricExporter struct { | |
aggregationSelector metric.AggregationSelector | ||
} | ||
|
||
// SetBatchProcessor sets a batch processor on the exporter | ||
func (e *MetricExporter) SetBatchProcessor(p modelpb.BatchProcessor) { | ||
e.processor = p | ||
} | ||
|
||
// Temporality returns the Temporality to use for an instrument kind. | ||
func (e *MetricExporter) Temporality(k metric.InstrumentKind) metricdata.Temporality { | ||
return e.temporalitySelector(k) | ||
|
@@ -67,6 +71,10 @@ func (e *MetricExporter) Aggregation(k metric.InstrumentKind) metric.Aggregation | |
} | ||
|
||
func (e *MetricExporter) Export(ctx context.Context, rm *metricdata.ResourceMetrics) error { | ||
if e.processor == nil { | ||
return nil | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. question: is it considered a valid state to run the exporter without a processor ? If not we should return an error here There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We may then end up with errors at boot time just because one export happened before the batch processor was setup. I don't think that's a good behavior either. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry I might be missing some context. If we end up with errors for early exports, why are we creating the exporter early ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I meant we may end up with early errors if we don't expect the processor to be nil, hence this check |
||
} | ||
|
||
batch := modelpb.Batch{} | ||
now := time.Now() | ||
|
||
|
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.
question: tied to the question below but if the processor is required we should leave it as part of the method instead of moving it to the config.
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.
Then it goes back to the issue we currently have, which is that we cannot create the metrics exporter (and therefore get a valid meter) until we have a batch processor.
But we also need a meter earlier, when we create the middlewares/interceptors.
We can't add a new exporter on an existing meter provider either. So this PR makes the batch processor optional to allow us configuring telemetry early, and extending it once we have a full boot.