Telemetry Processor (3): Envelope builder #3414
Draft
+865
−314
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…rs and logging - Introduced `EnvelopeBuilder` interface and implementations for log and span items, facilitating structured telemetry data handling. - Updated `InMemoryTelemetryBuffer` to utilize envelope builders for encoding and sending telemetry items, improving data transmission efficiency. - Enhanced logging capabilities within `InMemoryTelemetryBuffer` to capture encoding errors and buffer flushing events. - Added comprehensive tests for `InMemoryTelemetryBuffer`, ensuring correct behavior for item addition, flushing, and error handling.
- Updated the TelemetryBuffer interface and its implementations to rename the `clear` method to `flush`, aligning with its functionality of sending buffered items. - Adjusted the InMemoryTelemetryBuffer and DefaultTelemetryProcessor classes to reflect this change, ensuring consistent terminology across the codebase. - Modified related tests to use the new `flush` method, maintaining test integrity and clarity.
…TelemetryBuffer - Added a new `TelemetryBufferPolicy` class to define flushing behavior with configurable parameters such as `flushTimeout`, `maxBufferSizeBytes`, and `maxItemCount`. - Updated `InMemoryTelemetryBuffer` to utilize `TelemetryBufferPolicy`, allowing for more flexible buffer management based on item count and size. - Enhanced the SentryClient initialization to include a maximum item count for telemetry buffering, improving data handling efficiency.
…itialization - Updated SentryClient to utilize the new TelemetryBufferPolicy for managing telemetry item count, enhancing buffer management. - This change improves the efficiency of telemetry data handling by allowing for configurable item limits during client setup.
- Updated the initialization of defaultMaxBufferSizeBytes in TelemetryBufferPolicy to use a more concise expression, improving code clarity and consistency.
- Removed the policy parameter from InMemoryTelemetryBuffer initialization in SentryClient, streamlining the constructor and improving code clarity. - This change prepares for future enhancements related to telemetry processing.
…onfig - Renamed the TelemetryBufferPolicy class to TelemetryBufferConfig for improved clarity and consistency in naming. - Updated references in InMemoryTelemetryBuffer and related tests to reflect the new class name, ensuring seamless integration and functionality.
…ufferConfig - Replaced references to TelemetryBufferPolicy with TelemetryBufferConfig in InMemoryTelemetryBuffer and its tests, ensuring consistent naming and improved clarity. - Adjusted the constructor and internal logic to utilize the new configuration class for buffer management parameters.
…nhance telemetry context handling - Added DefaultTelemetryProcessorIntegration to set up the telemetry processor after Hub initialization, improving trace context extraction. - Enhanced PropagationContext with methods for managing trace context headers, ensuring better integration with telemetry processing. - Updated SentryClient to include the new integration, streamlining telemetry management during client setup. - Refactored envelope builders to support structured telemetry data handling for spans and logs, improving data transmission efficiency.
* feat(telemetry): Enhance InMemoryTelemetryBuffer with envelope builders and logging - Introduced `EnvelopeBuilder` interface and implementations for log and span items, facilitating structured telemetry data handling. - Updated `InMemoryTelemetryBuffer` to utilize envelope builders for encoding and sending telemetry items, improving data transmission efficiency. - Enhanced logging capabilities within `InMemoryTelemetryBuffer` to capture encoding errors and buffer flushing events. - Added comprehensive tests for `InMemoryTelemetryBuffer`, ensuring correct behavior for item addition, flushing, and error handling. * refactor(telemetry): Rename clear method to flush for consistency - Updated the TelemetryBuffer interface and its implementations to rename the `clear` method to `flush`, aligning with its functionality of sending buffered items. - Adjusted the InMemoryTelemetryBuffer and DefaultTelemetryProcessor classes to reflect this change, ensuring consistent terminology across the codebase. - Modified related tests to use the new `flush` method, maintaining test integrity and clarity. * feat(telemetry): Introduce TelemetryBufferPolicy and enhance InMemoryTelemetryBuffer - Added a new `TelemetryBufferPolicy` class to define flushing behavior with configurable parameters such as `flushTimeout`, `maxBufferSizeBytes`, and `maxItemCount`. - Updated `InMemoryTelemetryBuffer` to utilize `TelemetryBufferPolicy`, allowing for more flexible buffer management based on item count and size. - Enhanced the SentryClient initialization to include a maximum item count for telemetry buffering, improving data handling efficiency. * feat(telemetry): Integrate TelemetryBufferPolicy into SentryClient initialization - Updated SentryClient to utilize the new TelemetryBufferPolicy for managing telemetry item count, enhancing buffer management. - This change improves the efficiency of telemetry data handling by allowing for configurable item limits during client setup. * fix(telemetry): Correct defaultMaxBufferSizeBytes initialization - Updated the initialization of defaultMaxBufferSizeBytes in TelemetryBufferPolicy to use a more concise expression, improving code clarity and consistency. * refactor(telemetry): Simplify InMemoryTelemetryBuffer initialization - Removed the policy parameter from InMemoryTelemetryBuffer initialization in SentryClient, streamlining the constructor and improving code clarity. - This change prepares for future enhancements related to telemetry processing. * refactor(telemetry): Rename TelemetryBufferPolicy to TelemetryBufferConfig - Renamed the TelemetryBufferPolicy class to TelemetryBufferConfig for improved clarity and consistency in naming. - Updated references in InMemoryTelemetryBuffer and related tests to reflect the new class name, ensuring seamless integration and functionality. * refactor(telemetry): Update InMemoryTelemetryBuffer to use TelemetryBufferConfig - Replaced references to TelemetryBufferPolicy with TelemetryBufferConfig in InMemoryTelemetryBuffer and its tests, ensuring consistent naming and improved clarity. - Adjusted the constructor and internal logic to utilize the new configuration class for buffer management parameters. * feat(telemetry): Reject items exceeding max buffer size in InMemoryTelemetryBuffer - Implemented a check in InMemoryTelemetryBuffer to drop items that exceed the maximum buffer size, preventing oversized items from being added. - Updated tests to verify that items exceeding the buffer size are correctly rejected and not sent to the transport.
…TelemetryBuffer - Added a new `TelemetryBufferPolicy` class to define flushing behavior with configurable parameters such as `flushTimeout`, `maxBufferSizeBytes`, and `maxItemCount`. - Updated `InMemoryTelemetryBuffer` to utilize `TelemetryBufferPolicy`, allowing for more flexible buffer management based on item count and size. - Enhanced the SentryClient initialization to include a maximum item count for telemetry buffering, improving data handling efficiency.
…onfig - Renamed the TelemetryBufferPolicy class to TelemetryBufferConfig for improved clarity and consistency in naming. - Updated references in InMemoryTelemetryBuffer and related tests to reflect the new class name, ensuring seamless integration and functionality.
…ufferConfig - Replaced references to TelemetryBufferPolicy with TelemetryBufferConfig in InMemoryTelemetryBuffer and its tests, ensuring consistent naming and improved clarity. - Adjusted the constructor and internal logic to utilize the new configuration class for buffer management parameters.
…y processor - Updated `getOrCreateTraceContextHeader` method in `PropagationContext` to accept parameters for public key, segment name, release, environment, and traces sample rate, enhancing flexibility in trace context creation. - Introduced `DefaultTelemetryProcessorIntegration` to manage telemetry processing setup, ensuring proper integration with the Hub and enabling structured telemetry data handling. - Refactored envelope builders to utilize the new trace context handling, improving data transmission efficiency for spans and logs. - Added comprehensive tests for the new functionality, ensuring correct behavior and integration within the telemetry processing framework.
…st suite - Deleted the group of tests related to the telemetryProcessor in the SentryClient test suite, as they are no longer necessary. - This change simplifies the test suite and focuses on relevant functionality.
- Introduced a single instance of SampleRateFormat in PropagationContext to improve efficiency and readability. - Removed unnecessary imports from SimpleSpan and telemetry_processor_integration.dart, streamlining the codebase. - This change enhances clarity and reduces redundancy in the telemetry processing components.
![sentry[bot]](https://avatars.githubusercontent.com/u/1396951?s=60&v=4){kind=small}
Comment on lines
+55
to
+65
| return _traceContextHeader ??= SentryTraceContextHeader( | ||
| traceId, | ||
| publicKey, | ||
| release: release, | ||
| environment: environment, | ||
| transaction: segmentName, | ||
| sampleRate: _formatRate(tracesSampleRate), | ||
| sampleRand: _formatRate(sampleRand), | ||
| sampled: _sampled?.toString(), | ||
| ); | ||
| } |
There was a problem hiding this comment.
Bug: getOrCreateTraceContextHeader incorrectly caches the trace header. Subsequent calls for different segments within the same trace will reuse the first segment's header, leading to incorrect data.
Severity: HIGH | Confidence: High
🔍 Detailed Analysis
The getOrCreateTraceContextHeader() method in PropagationContext uses the ??= operator to cache the _traceContextHeader. This causes an issue when multiple spans from different segments within the same trace are processed. The PropagationContext instance is shared, so the header generated for the first segment is incorrectly reused for all subsequent segments. This results in sending trace context headers with the wrong transaction name for all segments after the first. While the captureSpan() method that triggers this flow is not yet fully implemented, this is a latent logical bug in the new envelope builder implementation.
💡 Suggested Fix
Remove the caching of the _traceContextHeader on the PropagationContext instance by removing the ??= operator. This will ensure a new trace context header is generated for each segment as needed, containing the correct segment-specific information.
🤖 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: packages/dart/lib/src/propagation_context.dart#L48-L65
Potential issue: The `getOrCreateTraceContextHeader()` method in `PropagationContext`
uses the `??=` operator to cache the `_traceContextHeader`. This causes an issue when
multiple spans from different segments within the same trace are processed. The
`PropagationContext` instance is shared, so the header generated for the first segment
is incorrectly reused for all subsequent segments. This results in sending trace context
headers with the wrong transaction name for all segments after the first. While the
`captureSpan()` method that triggers this flow is not yet fully implemented, this is a
latent logical bug in the new envelope builder implementation.
Did we get this right? 👍 / 👎 to inform future reviews.
Reference ID: 7708879
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| publicKey: options.parsedDsn.publicKey, | ||
| tracesSampleRate: options.tracesSampleRate, | ||
| segmentName: span.segmentSpan.name, | ||
| ), |
There was a problem hiding this comment.
Bug: Trace context uses wrong traceId from current propagation context
The trace context factory uses hub.scope.propagationContext to build the header, which means it uses the CURRENT propagation context's traceId rather than the span's actual traceId. When spans from an older trace remain in the buffer and a new trace starts (via resetTrace()), the propagation context's traceId changes. When the buffer flushes, spans from the old trace get an envelope with the new trace's traceId in the header, causing a mismatch between the envelope header and the span data inside. This could lead to incorrect trace correlation on the Sentry backend.
Flagged per review rules: this is a logic bug affecting correctness.
Additional Locations (1)
- Added new `SentrySpanV2` interface and implementations including `RecordingSentrySpanV2`, `NoOpSentrySpanV2`, and `UnsetSentrySpanV2` to support advanced span management. - Updated existing classes to utilize the new span API, ensuring compatibility with the telemetry processing framework. - Refactored related methods in `Hub`, `Scope`, and telemetry components to integrate the new span functionality, improving overall telemetry data handling. - Removed deprecated span classes and cleaned up imports to streamline the codebase.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
📜 Description
Adds the envelope builder implementation to the buffer
#skip-changelog
💡 Motivation and Context
Part of #3333
💚 How did you test it?
Unit test
📝 Checklist
sendDefaultPiiis enabled🔮 Next steps