Otel improvements#7798
Conversation
| /// <summary> | ||
| /// Controls which message payloads are promoted to span attributes. | ||
| /// </summary> | ||
| public enum MessagePayloadAsTag |
There was a problem hiding this comment.
Maybe we want an option to only log the payload on error
| /// as <c>nservicebus.message.{PropertyName}</c>. | ||
| /// Defaults to <see cref="MessagePayloadAsTag.None"/>. May expose sensitive data and incurs reflection cost. | ||
| /// </summary> | ||
| public MessagePayloadAsTag MessagePayloadAsTag { get; set; } |
There was a problem hiding this comment.
Do we want to split this in 2 properties one for incoming and one for outgoing.
| /// as <c>nservicebus.message.{PropertyName}</c>. | ||
| /// Defaults to <see cref="MessagePayloadAsTag.None"/>. May expose sensitive data and incurs reflection cost. | ||
| /// </summary> | ||
| public MessagePayloadAsTag MessagePayloadAsTag { get; set; } |
There was a problem hiding this comment.
Do we want to have a similar option to log the payload in our regular logs?
| using System.Threading.Tasks; | ||
| using Pipeline; | ||
|
|
||
| class IncomingMessagePayloadToTagsBehavior : IBehavior<IIncomingLogicalMessageContext, IIncomingLogicalMessageContext> |
There was a problem hiding this comment.
Do we need a behavior? Aren't we already settings tags already in core? If so, that likely can be extended instead.
| { | ||
| var json = JsonSerializer.Serialize(instance, instance.GetType()); | ||
| var base64 = Convert.ToBase64String(Encoding.UTF8.GetBytes(json)); | ||
| activity.SetTag("nservicebus.message.body", base64); |
There was a problem hiding this comment.
I don't think it should always use base64. If the payload if text, then it should just be utf8.
Second, we should NOT do extra (de)serialization. We should do this where we already have access to byte[] and then write this either as utf8 string or base64 string. To decide that we have probe for \0, force if via config (log payload as base64 or utf8), or assume utf8, catch exception on GetBytes and fallback to base64 encoding, maybe even another option.
There was a problem hiding this comment.
FYI As part of the assembly scanning efforts we have tried to sort as much as possible the paths that require dynamically referenced code. The JSonSerializer usage here would regress on that because it introduces another method usage that is not trimming friendly
[System.Diagnostics.CodeAnalysis.RequiresDynamicCode("JSON serialization and deserialization might require types that cannot be statically analyzed and might need runtime code generation. Use System.Text.Json source generation for native AOT applications.")]
[System.Diagnostics.CodeAnalysis.RequiresUnreferencedCode("JSON serialization and deserialization might require types that cannot be statically analyzed. Use the overload that takes a JsonTypeInfo or JsonSerializerContext, or make sure all of the required types are preserved.")]
public static string Serialize(object? value, Type inputType, System.Text.Json.JsonSerializerOptions? options = default);we are planning to add some kind of "regression" approval soon that should capture things like that in the future
While I'm here, I would like to add something else. I’m concerned about adding the full message body as an activity tag.
The OTel messaging semantic conventions define messaging.message.body.size, but that is only the size of the body in bytes, not the body content itself. I don’t see a standard semantic convention for attaching the full message payload to a span attribute.
Adding the full body as nservicebus.message.body, even base64 encoded, seems risky:
- high-cardinality span attributes
- potentially large telemetry payloads
- possible PII/secrets leakage
- increased indexing/storage cost in observability backends
- base64 does not provide sanitization
But I'm not in the weeds as you are so I might miss important context.
| // Append destination to span name per OTel messaging spec: {operation} {destination} | ||
| // Only for send/reply (unicast); publish destination is already set at span creation. | ||
| if (activityFactory.Options.UseMessageDestinationInSpanNames | ||
| && operations.Length > 0 | ||
| && operations[0].AddressTag is UnicastAddressTag unicastTag | ||
| && outgoingMessage.Headers.TryGetValue(Headers.MessageIntent, out var intentStr) | ||
| && intentStr is "Send" or "Reply") | ||
| { | ||
| activity.DisplayName = $"{activity.DisplayName} {unicastTag.Destination}"; | ||
| } |
There was a problem hiding this comment.
This likely should be moved to the ActivityDecorator
| && operations.Length > 0 | ||
| && operations[0].AddressTag is UnicastAddressTag unicastTag | ||
| && outgoingMessage.Headers.TryGetValue(Headers.MessageIntent, out var intentStr) | ||
| && intentStr is "Send" or "Reply") |
There was a problem hiding this comment.
THis should not use magic strings
| MergeDispatchProperties(publishContext, options.DispatchProperties); | ||
|
|
||
| using var activity = activityFactory.StartOutgoingPipelineActivity(ActivityNames.OutgoingEventActivityName, ActivityDisplayNames.PublishEvent, publishContext); | ||
| var publishDisplayName = activityFactory.Options.UseMessageDestinationInSpanNames |
There was a problem hiding this comment.
This likely should be in ActivityDecorator
|
|
||
| activity.DisplayName = ActivityDisplayNames.ProcessMessage; | ||
| activity.DisplayName = Options.UseMessageDestinationInSpanNames | ||
| ? $"{ActivityDisplayNames.ProcessOperation} {context.ReceiveAddress}" |
There was a problem hiding this comment.
Shouldn't we append the incoming message type? Like the 1st type from EnclodedMessageTypes?
…opagator (#7820) * initial migration to the DistributedContextPropagator for W3C compatibility * new propagator with backwards compatiblity tests * fixing the startnewtrace header progagation * refactor ContextPropagation.cs * small tweak * warning fixes * att tests reflect supported baggage and tracestate formats
Activity baggage with a null value used to throw ArgumentNullException during context propagation (Uri.EscapeDataString(null)). The switch to DistributedContextPropagator on this branch fixes the root cause; this test pins the behavior so it cannot regress.
Regression test for null baggage value (#6983)
…ards compatible until v11) (#7825) * ✨ Make DistributedContextPropagator opt-in via AppContext switch The switch to System.Diagnostics.DistributedContextPropagator changes the OpenTelemetry baggage wire format (W3C OWS encoding + whitespace trimming), which is breaking on rolling upgrades. Keep the legacy percent-encoded propagator as the default and gate the new propagator behind the NServiceBus.Core.OpenTelemetry.UseDistributedContextPropagator AppContext switch (default in v11). All temporary code (switch plumbing + legacy propagator) lives in obsolete_v11.cs so v11 cleanup is a single file deletion plus removing the two delegation blocks in ContextPropagation.cs. Follows the existing AppContextSwitches.UseV2DeterministicGuid / PreObsolete pattern. - ContextPropagation: delegate to the legacy propagator unless the switch is on - obsolete_v11.cs: switch + byte-for-byte revert of the pre-10.3 propagator - Tests asserting the new W3C format enable the switch per-fixture - New ContextPropagationDefaultBehaviorTests locks in legacy default behavior - Acceptance baggage assertion reverted to the legacy default wire format Span naming (UseMessageDestinationInSpanNames) is already opt-in and unchanged. * Refactor: Replace `ObsoleteV11` with `LegacyContextPropagation` * Add test to verify null baggage value does not throw in legacy propagator * Added comments about preserve legacy baggage handling behavior when escaping and trimming values * Add test to verify that we are preserving whitespace in legacy propagator baggage values. * add comments to clarify intent behind legacy propagator handling * update ContextPropagationCompatibilityTests to use correct LegacyContextPropagation delegates * rename ContextPropagationTests to LegacyContextPropagationTests and remove outdated baggage handling tests * allow changing the propagator implementation at runtime --------- Co-authored-by: Tomasz Masternak <tomasz.masternak@gmail.com>
4 participants
No description provided.