[WIP] perf: serialize telemetry batches on shared thread pool#1883
[WIP] perf: serialize telemetry batches on shared thread pool#1883jpnurmi wants to merge 3 commits into
Conversation
Add a bounded thread pool that runs tasks in parallel and invokes completion callbacks in submission order. Add unit coverage for ordered parallel execution.
Add an internal telemetry lifecycle layer for logs and metrics startup, shutdown, force flush, and crash-safe flush. Keep the existing logs and metrics behavior behind the shared coordination API.
Let the telemetry lifecycle own a shared serialization pool for enabled telemetry batchers. Use the pool for log and metric batch serialization while keeping completion ordered through the batcher flush lifecycle.
Instructions and example for changelogPlease add an entry to Example: ## Unreleased
### Features
- serialize telemetry batches on shared thread pool ([#1883](https://github.com/getsentry/sentry-native/pull/1883))If none of the above apply, you can opt out of this check by adding |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 2996a56. Configure here.
| sentry__logs_flush_crash_safe(); | ||
| sentry__metrics_flush_crash_safe(); | ||
| sentry__threadpool_flush(g_telemetry_pool); | ||
| } |
There was a problem hiding this comment.
Crash flush waits on thread pool
High Severity
sentry__telemetry_flush_crash_safe now calls sentry__threadpool_flush, which takes a mutex and waits on a condition variable. Crash backends invoke this from signal/exception handlers, where those primitives are unsafe and can hang forever if a pool worker is the crashing thread or already holds the lock, blocking crash report capture.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 2996a56. Configure here.
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| sentry__threadpool_flush(batcher->threadpool); | ||
| } | ||
|
|
||
| void | ||
| sentry__batcher_flush_crash_safe(sentry_batcher_t *batcher) | ||
| { | ||
| // Check if batcher is initialized | ||
| sentry__atomic_store(&batcher->crash_flush, 1); | ||
| const long state = sentry__atomic_fetch(&batcher->thread_state); | ||
| if (state == SENTRY_BATCHER_THREAD_STOPPED) { | ||
| return; |
There was a problem hiding this comment.
Bug: A race condition during crash handling may cause in-flight tasks to use the network transport instead of the crash-safe disk write, potentially losing pre-crash telemetry.
Severity: LOW
Suggested Fix
To prevent the race condition, either flush the threadpool before setting the crash_flush flag to ensure no in-flight tasks can race, or set the crash_flush flag atomically before any signal handling begins. This will guarantee that all tasks completing during a crash will correctly write to disk.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: src/sentry_batcher.c#L493-L506
Potential issue: During a crash, the system attempts to flush pending telemetry to disk.
A race condition exists where a task that was already in-flight before the crash might
complete its execution before the `crash_flush` flag is set by the crash handler. This
task would then incorrectly attempt to send its data over the network via
`sentry__transport_send_envelope()` instead of writing it to disk. This could lead to
the loss of telemetry data immediately preceding the crash, as the network transport may
not be crash-safe.
Also affects:
src/sentry_batcher.c:173~188
Did we get this right? 👍 / 👎 to inform future reviews.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #1883 +/- ##
==========================================
+ Coverage 75.18% 75.27% +0.08%
==========================================
Files 91 92 +1
Lines 21379 21608 +229
Branches 3764 3810 +46
==========================================
+ Hits 16074 16265 +191
- Misses 4477 4515 +38
Partials 828 828 🚀 New features to boost your workflow:
|
1 participant
Warning
WIP 🚧🔨⏳⛔
Close: #1862