feat(span-first): Refactor telemetry spans to SentrySpanV2
#3422
Conversation
- Updated span-related classes to implement SentrySpanV2, enhancing the telemetry system's structure and consistency. - Replaced instances of Span with SentrySpanV2 across Hub, NoOpHub, and related classes to unify span handling. - Introduced new span types: RecordingSentrySpanV2, NoOpSentrySpanV2, and UnsetSentrySpanV2 for improved span management. - Removed obsolete span classes and methods, streamlining the codebase and enhancing clarity. - Updated tests to reflect changes in span handling, ensuring robust coverage for new implementations.
…pdate DefaultTelemetryProcessor to remove outdated TODO comment regarding item type.
|
cursor review |
|
… type safety - Updated OnFlushCallback type parameter from S to T for improved clarity. - Enhanced error logging to include runtime type information and stack traces for better debugging. - Simplified item count increment logic for consistency.
- Changed abstract class declaration to use 'abstract base' for better type safety. - Updated InMemoryTelemetryBuffer and GroupedInMemoryTelemetryBuffer to use 'final' for class declarations, enhancing immutability. - Simplified error handling in the onFlush method by using a more generic Future type. - Improved item storage logic in GroupedInMemoryTelemetryBuffer by utilizing putIfAbsent for clarity. - Removed unnecessary imports from telemetry_buffer.dart to streamline the codebase.
| RecordingSentrySpanV2({ | ||
| required String name, | ||
| required SentryId defaultTraceId, | ||
| required SpanEndedCallback onSpanEnded, | ||
| required SdkLogCallback log, | ||
| required ClockProvider clock, | ||
| required RecordingSentrySpanV2? parentSpan, | ||
| }) : _spanId = SpanId.newId(), | ||
| _parentSpan = parentSpan, | ||
| _name = name, | ||
| _clock = clock, | ||
| _onSpanEnded = onSpanEnded, | ||
| _log = log, | ||
| _startTimestamp = clock(), | ||
| _traceId = parentSpan?.traceId ?? defaultTraceId { | ||
| _segmentSpan = parentSpan?.segmentSpan ?? this; | ||
| } |
There was a problem hiding this comment.
instead of a hub we should instead opt for passing only the required dependencies
- Updated span-related classes to replace 'defaultTraceId' with 'traceId' and 'onSpanEnded' with 'onSpanEnd' for consistency in naming conventions. - Enhanced logging in RecordingSentrySpanV2 to include warnings for invalid span states, improving error tracking. - Removed unnecessary imports from sentry_client.dart to streamline the codebase. - Updated tests to reflect changes in span initialization and logging, ensuring robust coverage for new implementations.
- Added a warning log for invalid span states in the captureSpan method, improving error tracking and debugging capabilities. - Removed redundant telemetryProcessor call for NoOpSentrySpanV2, streamlining span handling logic.
- Changed the declaration of MutableAttributesMixin from 'abstract mixin class' to 'mixin', enhancing code clarity and consistency in the telemetry attributes implementation.
|
cursor review |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
packages/dart/lib/src/telemetry/processing/telemetry_processor_integration.dart
Outdated
Show resolved
Hide resolved
- Replaced imports from 'telemetry/telemetry.dart' with 'telemetry/span/sentry_span_v2.dart' across multiple files to streamline the codebase and enhance clarity in span management. - Updated span-related classes to utilize the new SentrySpanV2 structure, improving consistency in telemetry span handling. - Removed obsolete 'attributes_mixin.dart' and 'telemetry.dart' files, consolidating span functionality into a more cohesive structure. - Adjusted tests to reflect changes in span initialization and imports, ensuring robust coverage for new implementations.
…key extraction - Updated error logging in _BaseInMemoryTelemetryBuffer to include the type parameter T for improved clarity in encoding failures. - Moved the spanGroupKeyExtractor declaration in DefaultTelemetryProcessorIntegration to a more appropriate location, ensuring better organization and visibility for testing purposes.
- Updated the createSpan method in telemetry_processor_integration_test.dart, telemetry_processor_test.dart, and span_test.dart to include spanId initialization using SpanId.newId(). - This change ensures consistent span identification across test cases, enhancing the reliability of telemetry span handling.
![sentry[bot]](https://avatars.githubusercontent.com/u/1396951?s=60&v=4){kind=small}
…ntification - Modified the spanGroupKeyExtractor in DefaultTelemetryProcessorIntegration to utilize segmentSpan.spanId instead of spanId, enhancing the accuracy of span grouping in telemetry processing.
…entification - Updated the createSpan method in scope_test.dart to include spanId initialization using SpanId.newId(), ensuring uniformity in span identification across tests.
…unctionality - Updated import paths in sentry_options.dart to reflect new structure. - Introduced new classes for telemetry buffering and processing, including TelemetryBuffer, InMemoryTelemetryBuffer, and DefaultTelemetryProcessor, enhancing the management of telemetry data. - Added TelemetryBufferConfig for configurable buffer settings. - Streamlined test imports to align with the new class structure, ensuring consistency across the codebase.
- Rearranged member variable declarations in _BaseInMemoryTelemetryBuffer to enhance code readability and maintainability. This change does not affect functionality but improves the overall structure of the class.
| } | ||
|
|
||
| if (encoded.length > _config.maxBufferSizeBytes) { | ||
| _logger( |
| abstract class TelemetryProcessor { | ||
| /// Adds a span to the buffer. | ||
| void addSpan(Span span); | ||
| void addSpan(RecordingSentrySpanV2 span); |
There was a problem hiding this comment.
Only the concrete type here? If so, have you thought about naming the protocol ISpanV2 and the RecordingSentrySpanV2 just SentrySpanV2? This would be in line with how the SDK was structured before, so it's easier to understand for users.
There was a problem hiding this comment.
the user still only interacts with the interface SentrySpanV2 e.g startSpan only returns and accepts SentrySpanV2 - all concrete types are marked as internal
internally for the telemetry processing, etc... allowing only the real recording span makes sense imo since supporting the whole interface with other concrete types (noop, unset) just adds extra if/switch checks everywhere that don't really add much value - imo we should just handle them at the startSpan level which we already do
we'd just litter the processor/buffer/scope and any other place that requires a 'real' span with unnecessary checks
e.g noop spans should not be serialized and be processed to the buffer so there is no reason to support this use case
we only have the span interface because of the requirement to have the UnsetSpan for startSpan otherwise I'd just model it as a concrete class SentrySpanV2 because realistically there is and will only be one "real" span implementation
have you thought about naming the protocol ISpanV2 and the RecordingSentrySpanV2 just SentrySpanV2
ideally I'd like to have the overall interface name just be SentrySpan - but we can't use it right now since it's already used in the transaction models
I matched the 'recording span' with what Sentry JS and general OTEL implementations name these "real" spans.
fwiw the user won't see RecordingSentrySpanV2 anyway
since it's also a brand new API we can deviate a little bit - personally I'm not a big fan of putting I in front of interfaces and rather have the concrete implementations be specific about what it is.
RecordingSentrySpan -> sentry span that is actively recording data
Updated the documentation for the attributes getter to clarify that it returns a read-only view using Map.unmodifiable and that the map must not be mutated.
📜 Description
This is mostly a refactor PR but I will go into details what has been changed
Span:
Span->SentrySpanV2UnsetSpan->UnsetSentrySpanV2NoOpSpan->NoOpSentrySpanV2File structure:
telemetrydirectorytelemetry/spanwhere span telemetry istelemetry/processingwhere processor and buffer istelemetry/log,telemetry/metric, etc...Telemetry Processor:
sentry_clientto separate integrationBuffer:
InMemoryTelemetryBufferandGroupedInMemoryTelemetryBufferJsonEncodableand instead use a generic type encoding function that's passed in the constructor💡 Motivation and Context
Span First
💚 How did you test it?
Unit tests
📝 Checklist
sendDefaultPiiis enabled🔮 Next steps
#skip-changelog