Fix: Rabbitmq7 header injection overwrites user supplied basic properties #6730
Conversation
|
@johang88: Thanks for this! We'll take a look, run all tests, and merge it if all goes well. |
| var targetType = typeof(TBasicProperties).Assembly.GetType("RabbitMQ.Client.BasicProperties")!; | ||
| var parameterType = typeof(TBasicProperties).Assembly.GetType("RabbitMQ.Client.IReadOnlyBasicProperties")!; | ||
|
|
||
| var constructor = targetType.GetConstructor([parameterType])!; | ||
|
|
||
| var createBasicPropertiesMethod = new DynamicMethod( | ||
| $"TypeActivator_{targetType.Name}_{parameterType.Name}", | ||
| targetType, | ||
| [parameterType], | ||
| typeof(DuckType).Module, | ||
| true); | ||
|
|
||
| var il = createBasicPropertiesMethod.GetILGenerator(); | ||
| il.Emit(OpCodes.Ldarg_0); | ||
| il.Emit(OpCodes.Newobj, constructor); | ||
| il.Emit(OpCodes.Ret); | ||
| Activator = (Func<object, object>)createBasicPropertiesMethod.CreateDelegate(typeof(Func<object, object>), targetType); |
There was a problem hiding this comment.
Unfortunately it looks like this is throwing an exception when attempting to run the tests / sample
[ERR] Exception occurred when calling the CallTarget integration continuation. System.TypeInitializationException: The type initializer for 'Datadog.Trace.ClrProfiler.AutoInstrumentation.RabbitMQ.CachedBasicPropertiesHelper`1' threw an exception.
---> System.ArgumentException: Cannot bind to the target method because its signature is not compatible with that of the delegate type.
at System.Delegate.CreateDelegateNoSecurityCheck(Type type, Object target, RuntimeMethodHandle method)
at System.Reflection.Emit.DynamicMethod.CreateDelegate(Type delegateType, Object target)
at Datadog.Trace.ClrProfiler.AutoInstrumentation.RabbitMQ.CachedBasicPropertiesHelper`1..cctor()
| } | ||
| else if (state.State.DuckIs<IReadOnlyBasicProperties>()) | ||
| { | ||
| returnValue = CachedBasicPropertiesHelper<TReturn>.CreateHeaders(state.State!); |
There was a problem hiding this comment.
I wonder if we need to preserve / copy headers here?
Haven't tested it out if that is necessary though
There was a problem hiding this comment.
I'll take a look and see if I can sort it out. We definitely need to copy the basic properties and headers as they will be dropped completely from the message otherwise. For example the current implementation completely breaks our rabbitmq message sender if instrumentation is enabled as the messageid, timestame, persistance settings will be removed.
There was a problem hiding this comment.
Thank you!
And let us know if need help on this as we definitely want to fix this
There was a problem hiding this comment.
I have added a commit that should fix the activator, not super happy about the cast but also not sure how to get rid of it. But I suppose optimizations can come after it's working properly :)
Note: Created as draft as I have not actually run or tested the changes 😶
Summary of changes
Fixes the header injection so that basic properties are preserved from the supplied argument list in case
PopulateBasicPropertiesHeadersreturns null which happens if the basic properties argument is writable or if no changes had to be made to it.Reason for change
Fixes a bug that would remove all user supplied basic properties.
Implementation details
Stores the basicproperties argument in the active scope and retrieves it a null return value is encountered. Changed
CachedBasicPropertiesHelperto call the copy constructor that takes aIReadOnlyBasicPropertiesas argument.Test coverage
Added basic properties to one of the rabbitmq7 test methods as well as assertions.
Other details
Fixes #6723