feat: config for custom batch span processor #229
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #229 +/- ##
==========================================
+ Coverage 57.80% 58.37% +0.57%
==========================================
Files 69 69
Lines 2737 2746 +9
==========================================
+ Hits 1582 1603 +21
+ Misses 1085 1074 -11
+ Partials 70 69 -1 ☔ View full report in Codecov by Sentry. |
|
Oh I missed some places eg. |
|
|
||
| exporter, err := exporterFactory() | ||
| if err != nil { | ||
| log.Fatal(err) | ||
| } | ||
|
|
||
| sp := modbsp.CreateBatchSpanProcessor( | ||
| cfg.GetTelemetry() != nil && cfg.GetTelemetry().GetMetricsEnabled().GetValue(), // metrics enabled |
There was a problem hiding this comment.
Just to understand this more, why was this conditional to run BatchSpanProcesor until now? What I am trying to understand is what would happen if customBsp is false and metrics are enabled? Will non-modified batchSpanProcessor work?
There was a problem hiding this comment.
The only reason for the custom BatchSpanProcessor(BSP) is to be able to collect span metrics - received, dropped and unsampled - as they hit the exporter. So if metrics is disabled, there's no need for it.
As if this PR if custom BSP is false and metrics are enabled, we use the stock BSP. I think this behavior will remain even in the recover from panic PR. And I'm thinking I will remove the metrics enabled requirement in that PR so that we can recover even if metrics are disabled and panic happens when exporting the traces.
Description
We need to be able to pick whether we use out customer bsp https://github.com/hypertrace/goagent/tree/main/instrumentation/opentelemetry/batchspanprocessor or use the stock one in the opentelemetry go repo.
Testing
Unit and local tests
Checklist: