feat: Add export_unsampled option to BatchSpanProcessor for recording but unsampled spans #3139
Conversation
|
@cijothomas 👋 This repository doesn't have Copilot instructions. With Copilot instructions, I can understand the repository better, work faster and produce higher quality PRs. I can generate a .github/copilot-instructions.md file for you automatically. Click here to open a pre-filled issue and assign it to me. I'll write the instructions, and then tag you for review. |
|
Watching along to see how far Copilot can get! |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3139 +/- ##
=======================================
+ Coverage 80.6% 80.7% +0.1%
=======================================
Files 126 126
Lines 22195 22342 +147
=======================================
+ Hits 17898 18042 +144
- Misses 4297 4300 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Co-authored-by: cijothomas <5232798+cijothomas@users.noreply.github.com>
Co-authored-by: cijothomas <5232798+cijothomas@users.noreply.github.com>
Copilot
AI
changed the title
…ntire config - Remove config field from BatchSpanProcessor structs (both sync and async) - Add export_unsampled bool field directly to structs - Extract export_unsampled from config during construction - Update sampling logic to use direct field access - Fix tests to use sampled spans to maintain expected behavior - Add Clone trait to BatchConfig for compatibility This addresses the reviewer feedback to minimize unrelated changes while preserving the export_unsampled functionality. Co-authored-by: cijothomas <5232798+cijothomas@users.noreply.github.com>
| max_export_batch_size: OTEL_BSP_MAX_EXPORT_BATCH_SIZE_DEFAULT, | ||
| max_export_timeout: OTEL_BSP_EXPORT_TIMEOUT_DEFAULT, | ||
| max_concurrent_exports: OTEL_BSP_MAX_CONCURRENT_EXPORTS_DEFAULT, | ||
| export_unsampled: false, |
There was a problem hiding this comment.
Could we also make this flag configurable via an environment variable, consistent with other BSP knobs? something like OTEL_BSP_EXPORT_UNSAMPLED
There was a problem hiding this comment.
OTel spec has general put a halt of adding new env variables, as OTel is moving towards new configuration model. I am not sure if we should add another env variable now..
cijothomas
changed the title
|
@copilot Please make sure all lint failures are addressed. Running |
Co-authored-by: cijothomas <5232798+cijothomas@users.noreply.github.com>
Fixed! I've run |
|
Thanks for getting this out so quick, seems like the CI might of gotten stuck. Probably just needs to be restarted ? |
|
Specs PR is not yet merged - open-telemetry/opentelemetry-specification#2986. Do we plan to introduce before that? |
Progress Summary
All lint failures have been addressed by running
cargo fmt --all. The formatting changes include:The feature implementation is complete and all code style issues are resolved.
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.