Using otlphttp exporter for self metrics#2093
Using otlphttp exporter for self metrics#2093rafaelwestphal wants to merge 50 commits intomasterfrom
Conversation
|
Why is this using otlphttp instead of otlpgrpc? |
otlpgrpc doesn't allow us to set the user agent, but otlphttp does. we are using otlphttp until that gets implemented. iirc from the initial analysis, implementing that was not trivial |
rafaelwestphal
requested review from
a team,
franciscovalentecastro and
ridwanmsharif
and removed request for
a team
| return ConvertToOtlpExporter(pipeline, ctx, false, true) | ||
| } | ||
|
|
||
| func ConvertToOtlpExporter(pipeline otel.ReceiverPipeline, ctx context.Context, isPrometheus bool, isSystem bool) otel.ReceiverPipeline { |
There was a problem hiding this comment.
It is great that now all logic to setup the OTLP exporter instead of the GCM exporter is contained within ConvertToOtlpExporter!
Question
I want to followup on this comment made above :
The problem is that the transformation is different depending if the current exporter is configured as a system or not. If I just declare it as OTLP, I don' t know how to properly setup the pipeline later. It can either be a GMP, GCM system or GCM Otel exporter.
When the migration to the OTLP exporter is fully completed and we remove all the other exporters (System, OTel , GMP) :
- How are we going to determine which combination of processors (isSystem, isPrometheus) should be applied to each metric pipeline ?
- Should we add another ReceiverPipeline "property" to differentiate them ?
- When we remove the
GMPexporter, how is the functionConvertPrometheusExporterToOtlpExportergoing to be renamed/refactored ?
I bring up this questions, since i feel the solution of using a "converter" (ConvertToOtlpExporter) is considering only the intermediate state where GCM is the main exporter, but will need further refactoring when those are removed. My suggestion here aims to find a solution that won't need any refactoring later.
Suggestion
One idea is to have 3 exporter types which help determine the specific processors needed for each pipeline (while keeping the possibility to store the logic in one place ) :
- System_OTLP
- GMP_OTLP
- OTLP
This keeps the abstraction of a "ReceiverPipeline" while also having a way to differentiate the specific requirements of each metric pipeline.
There was a problem hiding this comment.
Synced offline with @rafaelwestphal. Mentioned eventually we are going to consolidate all OTLP exporter processor logic when the migration is complete. Using a ConvertToOtlpExporter is considered an intermediate step since it enables development to happen for each receiver separately.
franciscovalentecastro
left a comment
franciscovalentecastro
left a comment
There was a problem hiding this comment.
LGTM modulo addressing the last comments and all integration tests pass.
confgenerator/confgenerator.go
Outdated
| if processors, ok := receiverPipeline.Processors["metrics"]; ok && expOtlpExporter { | ||
| receiverPipeline.Processors["metrics"] = append( | ||
| processors, | ||
| otel.MetricStartTime(), |
There was a problem hiding this comment.
[Requested change] Create an otlpExporterComponents (or otlpExporterProcessor) function that contains MetricStartTime() and also will serve for future additions of OTLP exporter logic.
confgenerator/confgenerator.go
Outdated
| } | ||
|
|
||
| expOtlpExporter := experimentsFromContext(ctx)["otlp_exporter"] | ||
| if processors, ok := receiverPipeline.Processors["metrics"]; ok && expOtlpExporter { |
There was a problem hiding this comment.
[Comment] Just as a general comment of our offline discussion, IMO here is the best place to consolidate all OTLP exporter logic (e.g. post-processors) since it resembles how otelSetLogNameComponents is unilaterally added in all logging pipelines.
I believe you can remove all uses of ConvertToOtlpExporter and add all the "OTLP" logic here. Please keep track of this ideas as the migration moves forward.
| - grpc.client.attempt.duration_count | ||
| - googlecloudmonitoring/point_count | ||
| interval/loggingmetrics_7: | ||
| interval: 1m |
There was a problem hiding this comment.
[Requested changed] Add a confgenerator/testdata case for self metrics to track future regressions on "OTLP exporter self metrics" pipelines.
|
[nit] Add a PR description with details about the changes. |
franciscovalentecastro
left a comment
franciscovalentecastro
left a comment
There was a problem hiding this comment.
I'll keep the approval (to not block the PR) modulo addressing the [Requested Change] above.
3 participants
Description
Related issue
How has this been tested?
Checklist: