Generating otlphttp endpoint when feature flag is enabled #2022
Conversation
Co-authored-by: Jeff Erbrecht <89024676+jefferbrecht@users.noreply.github.com>
confgenerator/confgenerator.go
Outdated
| } | ||
| // Check the Ops Agent receiver type. | ||
| if receiverPipeline.ExporterTypes[p.pipelineType] == otel.GMP { | ||
| if receiverPipeline.ExporterTypes[p.pipelineType] == otel.GMP || receiverPipeline.ExporterTypes[p.pipelineType] == otel.Otlp { |
There was a problem hiding this comment.
Is the new exporter intended to eventually be re-used for non-Prometheus pipelines as well?
If so, then I think this check is a bit of a time bomb, because there's no restriction on processors for non-Prometheus pipelines. If and when we decide to re-use the exporter for non-Prometheus, it would introduce a bug that we might not catch.
Otherwise, I think the new exporter type should be named something that's specific to Prometheus, i.e. OtlpGMP or something.
There was a problem hiding this comment.
The new exported is going to be re-used for non-Prometheus pipelines as well.
I changed the check here to prevent that problem from happening. This check should cover us for the scenarios:
- Prometheus receiver with GMP exporter
- Prometheus receiver with OTLP exporter
- OTLP receiver with GMP exporter
There was a problem hiding this comment.
What about OTLP receiver with OTLP exporter, but the OTLP receiver is configured in GMP mode? I think that's still not covered.
What if we add a new map adjacent to receiverPipeline.ExporterTypes that identifies the data model (GCM vs. GMP) for each pipeline/signal type? Today we assume that the data model is implied entirely by the exporter type, but with the OTLP exporter it's no longer true. The change you're proposing is potentially going down a rabbit hole where we'd need to exhaustively check every combination of receiver type and exporter type.
There was a problem hiding this comment.
Or actually, since the data model depends on the configuration in the case of the OTLP receiver, it might have to be a function rather than a map.
There was a problem hiding this comment.
I've reworked this part of the code and moved it to the validator.
PTAL
…1988) Co-authored-by: Francisco Valente Castro <1435136+franciscovalentecastro@users.noreply.github.com>
…dexporter` fix. (#2048)
Co-authored-by: Francisco Valente Castro <1435136+franciscovalentecastro@users.noreply.github.com>
Co-authored-by: Francisco Valente Castro <1435136+franciscovalentecastro@users.noreply.github.com>
Co-authored-by: Jeff Erbrecht <89024676+jefferbrecht@users.noreply.github.com>
confgenerator/otel/modular.go
Outdated
| } | ||
| } | ||
| if len(c.Extensions) > 0 { | ||
| extensionsList := []string{} |
There was a problem hiding this comment.
Please update extensionsList := []string{} to var extensionList []string
See: go/go-style/decisions#nil-slices
confgenerator/otel/modular.go
Outdated
| OTel ExporterType = iota | ||
| System | ||
| GMP | ||
| Otlp |
There was a problem hiding this comment.
We should stylize this abbrevation in all-caps similar to GMP
confgenerator/otel/modular.go
Outdated
| telemetryMap["logs"] = logs | ||
| } | ||
| } | ||
| if len(c.Extensions) > 0 { |
There was a problem hiding this comment.
This was added here but then is repeated down below, is this intentional?
There was a problem hiding this comment.
Thanks, bad code merge.
Description
Use new feature flag otlp_endpoint to replace Google managed Prometheus exporter with the otlphttp exporter for Prometheus metrics.
Related issue
b/435217702
How has this been tested?
Unit test has been added to verify that the configuration is being generated when the feature flag is enabled.
Checklist: