-
-
Notifications
You must be signed in to change notification settings - Fork 280
feat(span-first): Add traceLifecycle option, dsc to span and sampling to startSpan
#3429
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
- Introduced sampling decision logic in the Hub class to determine whether spans should be recorded based on a random sampling rate. - Updated the captureSpan method to return a Future, allowing for asynchronous handling of span captures. - Enhanced SentryTraceContextHeader to create headers from RecordingSentrySpanV2, improving telemetry context management. - Added a factory method for creating SentryTraceContextHeader from spans, facilitating better integration with the telemetry system. - Updated tests to reflect changes in span handling and sampling logic, ensuring robust coverage for new implementations.
|
…tations - Renamed the `dscFactory` parameter to `dscCreator` in the Hub and RecordingSentrySpanV2 classes for clarity. - Updated all references and implementations across the codebase to reflect this change. - Added a new `captureSpan` method in NoOpSentryClient to support the updated span handling. - Adjusted tests to accommodate the new parameter name.
- Added `SentryTraceLifecycle` enum to control trace data collection methods (streaming vs static). - Enhanced `SentrySamplingContext` to include span sampling context and lifecycle options. - Updated `RecordingSentrySpanV2` to support sampling decisions for root and child spans. - Modified `Hub` class to evaluate sampling decisions at the root span level, ensuring child spans inherit the sampling decision. - Added tests to verify sampling inheritance and behavior across nested spans. - Updated example application to utilize the new streaming trace lifecycle option.
- Refactored `captureSpan` method in `Hub`, `NoOpSentryClient`, and `SentryClient` to return void instead of FutureOr<void> for consistency. - Enhanced `SentrySamplingContext` with new constructors for transaction and span contexts, improving clarity in sampling decision evaluations. - Updated `SentryTracesSampler` to utilize the new sampling context structure, ensuring proper handling of static and streaming trace lifecycles. - Added comments to clarify the limitations of distributed trace support in the current implementation. - Adjusted tests to validate the new span handling and sampling context behavior, ensuring robust coverage for the updated functionality.
- Updated `SentrySamplingContext` to clarify its dual-mode functionality for transaction and span contexts, ensuring backwards compatibility. - Added runtime checks and comments to guide usage in static and streaming modes. - Included TODOs for future simplification once the legacy transaction API is removed.
- Changed expected error types in `SentrySamplingContext` tests from `AssertionError` to `StateError` for accessing `transactionContext` and `spanContext`. - Improved formatting in test cases for better readability. - Removed unnecessary blank lines in mock telemetry processor.
|
bugbot review |
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.
✅ Bugbot reviewed your changes and found no bugs!
- Updated the `SentryTraceContextHeader` class to use the correct sampling decision properties from the `span` object instead of the `hub.scope.propagationContext`. - Ensured that `sampleRand` and `sampled` are now accurately derived from the `span.samplingDecision`, improving the integrity of trace context handling.
- Modified the `captureSpan` method in the `Hub` class to return void instead of null for better clarity and consistency in handling no-op scenarios. - Ensured that the return type aligns with the recent refactor of span handling, improving overall code readability.
| /// Placeholder for streaming mode where transaction context is not used. | ||
| static final _unusedTransactionContext = | ||
| SentryTransactionContext('unused', 'unused'); | ||
|
|
||
| /// Placeholder for static mode where span context is not used. | ||
| static final _unusedSpanContext = SentrySpanSamplingContextV2('unused', {}); |
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.
we can't remove/change transactionContext because it's a breaking change so we have to add some mechanism for spanContext to co-exist with it but only have one active at a time
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.
LMK if there is a better option here
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.
I think it's fine, it's a straight forward solution as long as we have to support both.
- Changed the return type of the `captureSpan` method in `MockSentryClient` from `FutureOr<void>` to `void` to align with recent refactoring efforts. - This update enhances clarity and consistency in the method's behavior, following similar changes made in the `Hub` class.
- Modified the test for `SentryTraceContextHeader` to use the sampling decision from the span instead of the propagation context. - Updated the test case to reflect the new sampling decision structure, ensuring accurate testing of trace context handling.
…lemetry processor - Removed unnecessary imports from `SentryTraceContextHeader` to streamline the codebase. - Eliminated the unused `close` method in `MockTelemetryProcessor`, enhancing code clarity and maintainability.
startSpantraceLifecycle option, dsc to span and sampling to startSpan
| /// streaming (span-based) modes for backwards compatibility. The dual-mode | ||
| /// design with placeholder values and runtime checks is a temporary solution. | ||
| /// Once the legacy transaction API is removed, this class should be simplified | ||
| /// to only support the streaming mode with [SentrySpanSamplingContextV2]. |
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.
Thx for the explanation why this is here 👍
| /// Placeholder for streaming mode where transaction context is not used. | ||
| static final _unusedTransactionContext = | ||
| SentryTransactionContext('unused', 'unused'); | ||
|
|
||
| /// Placeholder for static mode where span context is not used. | ||
| static final _unusedSpanContext = SentrySpanSamplingContextV2('unused', {}); |
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.
I think it's fine, it's a straight forward solution as long as we have to support both.
packages/dart/lib/src/telemetry/span/recording_sentry_span_v2.dart
Outdated
Show resolved
Hide resolved
Added documentation for the attributes field.
- Replaced specific span creation methods with a unified `createSpan` method in telemetry tests, enhancing code clarity and reducing redundancy. - Updated the `Fixture` class to support optional `dscCreator` and `samplingDecision` parameters, streamlining span initialization.
- Replaced StateError throws with internalLogger error calls in the transactionContext and spanContext getters to enhance error reporting and maintain backwards compatibility. - This change provides clearer logging for developers when accessing context in incorrect modes.
- Modified the `fromRecordingSpan` factory method in `SentryTraceContextHeader` to accept `SentryOptions` and `replayId` as parameters instead of `Hub`. - Updated the `dscCreator` in the `Hub` class to reflect these changes, ensuring proper context propagation. - Adjusted related tests to use the new method signature, enhancing clarity and consistency in span creation.
- Eliminated tests that checked for StateError throws when accessing transactionContext and spanContext, as these checks are no longer necessary following recent error handling improvements. - This cleanup enhances the clarity of the test suite by focusing on relevant functionality and expected behaviors.
📜 Description
New Features
Dynamic Sampling Context (DSC) for the new SpanV2 system
SentryTraceContextHeader.fromRecordingSpan()Sampling for SpanV2
tracesSampleRateandtracesSampleroptionsNew
traceLifecycleoptionSentryTraceLifecycle.streaming- spans sent individually as they completeSentryTraceLifecycle.static- spans sent together when (legacy) transaction ends (default)Code Changes
RecordingSentrySpanV2 now uses factory constructors:
RecordingSentrySpanV2.root()- creates root span with sampling decisionRecordingSentrySpanV2.child()- creates child span inheriting from parentSentrySamplingContext updated:
forSpanV2()constructor for streaming modeforTransaction()constructor for static mode (legacy)Hub.captureSpan() now forwards spans to
SentryClient.captureSpan()SentryClient.captureSpan() adds spans to the telemetry processor
💡 Motivation and Context
Part of #3331
💚 How did you test it?
Unit tests
📝 Checklist
sendDefaultPiiis enabled🔮 Next steps
#skip-changelog