Started perf optimizations - #7917
Open
irinascurtu wants to merge 1 commit into
Open
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR introduces performance optimizations in the OpenTelemetry tracing instrumentation by skipping tag/event work when activities are created for propagation-only sampling (i.e., when Activity.IsAllDataRequested is false), while still ensuring context propagation remains correct.
Changes:
- Skip header-to-tag promotion when an activity is sampled as
PropagationData. - Bail out of recoverability activity updates when full activity data isn’t requested.
- Extend test infrastructure and add coverage for
PropagationDatasampling behavior.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/NServiceBus.Core/OpenTelemetry/Tracing/ActivityFactory.cs | Skips tag/event work for propagation-only activities; adjusts error recording and recoverability updates. |
| src/NServiceBus.Core/OpenTelemetry/Tracing/ActivityExtensions.cs | Updates comment describing the IsAllDataRequested guard for “expensive” work. |
| src/NServiceBus.Core.Tests/OpenTelemetry/Helpers/TestingActivityListener.cs | Allows configuring sampling result in tests (AllData vs PropagationData). |
| src/NServiceBus.Core.Tests/OpenTelemetry/ActivityFactoryTests.cs | Adds tests validating PropagationData behavior (no header tag promotion, no exception events/legacy tags, no recoverability updates). |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
226
to
230
| if (!exception.Data.Contains(ExceptionRecordedFlag)) | ||
| { | ||
| if (Options.ExceptionRecordingMode == ExceptionRecordingMode.Logs) | ||
| { | ||
| Logger.Error($"An exception occurred while executing '{activity.DisplayName}'.", exception); |
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
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.
Skip OTel enrichment work when
IsAllDataRequestedis falseIsAllDataRequested, is represented by https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/trace/api.md#isrecording
https://github.com/open-telemetry/opentelemetry-dotnet/blob/main/src/OpenTelemetry.Api/README.md#:~:text=a%20null%20check.-,Populate,-activity%20with%20tags
ActivityFactorywas only null-checking activities before doing enrichment work (promotingheaders to tags, recording exceptions, updating recoverability tags/
DisplayName), even when alistener sampled the activity as
PropagationData-only (IsAllDataRequested == false). In thatcase the work is wasted: nothing reads it.
This guards the expensive paths behind
IsAllDataRequested, matching the pattern already usedby
TryGetRecordingOutgoingPipelineActivity/TryGetRecordingIncomingPipelineActivity:PromoteHeadersToTags(header -> tag mapping on incoming messages) - skipped when notrequested. Context propagation (baggage/tracestate) stays unconditional since it's
correctness, not enrichment.
RecordError- the exception event (stack trace) and legacyotel.status_code/otel.status_descriptiontags are now guarded.SetStatus, theErrorTypetag, and theLogs-mode logging fallback stay unconditional.UpdateActivityFromRecoverabilityAction- now a no-op when not requested.No behavior change for fully-sampled or no-listener cases. Added a
PropagationDataSamplingtest class in
ActivityFactoryTests.cs(and a sampling-result parameter on the sharedTestingActivityListener, default unchanged) to cover the previously untestedPropagationDatapath.