Split Apollo trace/metrics exporter configs#8258
Conversation
✅ Docs preview readyThe preview is ready to be viewed. View the preview File Changes 0 new, 1 changed, 0 removedBuild ID: a7520cde1acd5e890e62f11d URL: https://www.apollographql.com/docs/deploy-preview/a7520cde1acd5e890e62f11d |
This comment has been minimized.
This comment has been minimized.
| ``` | ||
| telemetry: | ||
| apollo: | ||
| batch_processor: | ||
| scheduled_delay: 5s | ||
| max_export_timeout: 30s | ||
| max_export_batch_size: 512 | ||
| max_concurrent_exports: 1 | ||
| max_queue_size: 2048 | ||
| ``` | ||
|
|
||
| To: | ||
|
|
||
| ``` |
There was a problem hiding this comment.
can you add yaml to the right of the opening triple ticks? That'll let github format it as yaml. e.g.
here:
is:
number: 15vs.
here:
is:
number: 15
| # Config for Apollo OTLP metrics. Note that some metrics like config values have a non-configurable scheduled_delay. | ||
| otlp: | ||
| exporter: | ||
| scheduled_delay: 13s # only applies to realtime metrics (not config metrics) |
There was a problem hiding this comment.
Should we make this more obvious in the config? e.g. metrics.otlp.realtime.exporter.scheduled_delay?
There was a problem hiding this comment.
I don't want to split this config into 2 separate configs since we're already at 4 and that's a lot. And the max_export_timeout config applies to both realtime and non-realtime. I think I should just update the comment to be a bit more definite so it's obvious about what the scheduled_delay controls and doesn't control.
| }, | ||
| "max_queue_size": { | ||
| "default": 2048, | ||
| "description": "The maximum queue size to buffer spans for delayed processing. If the queue gets full it drops the spans. The default value of is 2048.", |
There was a problem hiding this comment.
typo: The default value of is --> The default value is
| Ok(builder.with_span_processor( | ||
| BatchSpanProcessor::builder(exporter, NamedTokioRuntime::new("apollo-tracing")) | ||
| .with_batch_config(self.batch_processor.clone().into()) | ||
| .with_batch_config(self.traces.otlp.exporter.clone().into()) // todo?? which one to pick |
There was a problem hiding this comment.
TODO here needs resolution
There was a problem hiding this comment.
Tough question. Looking at this through the lens that by default most folks on Router 2.x will be using OTLP tracing I'd lean in that direction. You could have some logic that decides based on the sampler which one will win (like if 50% or more OTLP, than choose that one). Can't wait to sunset the old Apollo trace exporter.
There was a problem hiding this comment.
Actually do we need to distinguish between OTLP and legacy for this config currently? Like maybe we can just have one Apollo tracing configuration that covers traces for both OTLP & usage reporting?
There was a problem hiding this comment.
Oops - I did decide that the traces OTLP setting is the right one to go with, since this is used only for OTLP, but I just didn't remove the comment.
There was a problem hiding this comment.
I've combined the two tracing configs now, so there shouldn't be any strange behaviour here.
timbotnik
left a comment
timbotnik
left a comment
There was a problem hiding this comment.
Aside from tests failing and one small nit, LGTM.
Co-authored-by: timbotnik <tim@apollographql.com>
4 participants
The config related to the exporting of Apollo metrics and traces has been separated so that the various configuration can be fine-tuned for each of the Apollo exporters. Each of Apollo OTLP traces, Apollo usage report traces, Apollo OTLP metrics, and Apollo usage report metrics now have their own config. The old
telemetry.apollo.batch_processorconfig will be used if these new config values are not specified. The configuration used will be shown in an info-level log on router startup.Checklist
Complete the checklist (and note appropriate exceptions) before the PR is marked ready-for-review.
Exceptions
Note any exceptions here
Notes
Footnotes
It may be appropriate to bring upcoming changes to the attention of other (impacted) groups. Please endeavour to do this before seeking PR approval. The mechanism for doing this will vary considerably, so use your judgement as to how and when to do this. ↩
Configuration is an important part of many changes. Where applicable please try to document configuration examples. ↩
A lot of (if not most) features benefit from built-in observability and
debug-level logs. Please read this guidance on metrics best-practices. ↩Tick whichever testing boxes are applicable. If you are adding Manual Tests, please document the manual testing (extensively) in the Exceptions. ↩