-
Notifications
You must be signed in to change notification settings - Fork 44
feat: default tail sampling #151
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR makes tail sampling the default behavior in fastrace, meaning spans are now always held until the root span finishes. The Config::tail_sampled() method is deprecated as a no-op since the feature is now always enabled. The semantics of span lifecycle methods are changed: Span::cancel() now triggers cancellation (previously triggered by drop_collect), and dropping the root span now triggers reporting (previously triggered by commit_collect).
- Renamed internal commands to reflect new semantics:
CommitCollect→ removed,DropCollect→CancelCollect, and a newDropCollectreplaces the oldCommitCollect - Added
canceledflag toActiveCollectorto track cancelled traces and prevent reporting them - Fixed a bug in SPSC sender where the new value could be pushed multiple times if the queue was full while draining pending values
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
tests/statically-disable/src/main.rs |
Removed .tail_sampled(false) call as it's now deprecated |
fastrace/tests/lib.rs |
Removed .tail_sampled(true) call as it's now the default |
fastrace/src/util/spsc.rs |
Renamed pending_messages to pending and fixed bug where value could be pushed twice when queue is full |
fastrace/src/util/command_bus.rs |
Updated copyright header and simplified WASM notify logic |
fastrace/src/span.rs |
Added Debug impl, updated cancel() docs, swapped commit_collect/drop_collect semantics, updated tests |
fastrace/src/local/local_span.rs |
Added Debug implementation for LocalSpan |
fastrace/src/lib.rs |
Updated documentation to clarify reporter invocation timing |
fastrace/src/collector/mod.rs |
Deprecated tail_sampled(), removed field from Config, updated docs |
fastrace/src/collector/global_collector.rs |
Swapped command names, added canceled field to ActiveCollector, updated command handling logic |
fastrace/src/collector/command.rs |
Renamed commands: CommitCollect → removed, DropCollect → CancelCollect, new DropCollect |
CHANGELOG.md |
Added entry documenting the deprecation |
AGENTS.md |
Updated documentation to reflect new tail sampling behavior and deprecated config option |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| fn notify(&self) { | ||
| #[cfg(not(target_family = "wasm"))] | ||
| { | ||
| self.notify_tx.try_send(()).ok(); | ||
| } | ||
| #[cfg(target_family = "wasm")] | ||
| { | ||
| Ok(()) | ||
| } | ||
| self.notify_tx.try_send(()).ok(); | ||
| } |
Copilot
AI
Dec 18, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The removal of the WASM-specific code path appears to be incorrect. The method notify() was previously a no-op on WASM targets (returning Ok(())), but now it calls self.notify_tx.try_send(()).ok() unconditionally, which will fail to compile on WASM targets because NotifySender doesn't have a notify_tx field when compiled for WASM (see line 118-119 where the field is conditionally compiled with #[cfg(not(target_family = "wasm"))]). This will cause a compilation error on WASM platforms.
No description provided.