From bfeb3ad8f13565abfbaeecd38170aa7ee3cf4462 Mon Sep 17 00:00:00 2001 From: Roni Dover Date: Wed, 22 Mar 2023 12:06:12 -0700 Subject: [PATCH 1/2] previously we only checked the methodinfo provided in the dispatch proxy which correlates to the interface method --- .../Stubs/DecoratedService.cs | 12 ++- .../TestTracingDecorator.cs | 21 +++++ .../Helpers/TracingDecorator.cs | 77 +++++++++++++++---- 3 files changed, 94 insertions(+), 16 deletions(-) diff --git a/src/OpenTelemetry.Instrumentation.Digma.Tests/Stubs/DecoratedService.cs b/src/OpenTelemetry.Instrumentation.Digma.Tests/Stubs/DecoratedService.cs index b36b8b3..ce0e16c 100644 --- a/src/OpenTelemetry.Instrumentation.Digma.Tests/Stubs/DecoratedService.cs +++ b/src/OpenTelemetry.Instrumentation.Digma.Tests/Stubs/DecoratedService.cs @@ -22,8 +22,11 @@ public void MethodJaggedAndMultiDimArraysParams(Action stateValidation, out stri bool[][][] jaggedArrayOfBools, short[,,,][,][,,] multiDimArrayOfShorts, long[,,][][,][] mixMultiDimAndJaggedArraysOfLongs ); -} + void MethodExplicitlyMarkedForTracingWithAttributes(Action action); +} + +[ActivitiesAttributes("att1:value1")] public class DecoratedService : IDecoratedService { [TraceActivity()] @@ -32,6 +35,13 @@ public void MethodExplicitlyMarkedForTracing(Action stateValidation) var v = Activity.Current; stateValidation(); } + + [TraceActivity()] + [ActivitiesAttributes("att1:value1")] + public void MethodExplicitlyMarkedForTracingWithAttributes(Action stateValidation) + { + stateValidation(); + } [TraceActivity()] public async Task AsyncMethodExplicitlyMarkedForTracing(Action stateValidation) diff --git a/src/OpenTelemetry.Instrumentation.Digma.Tests/TestTracingDecorator.cs b/src/OpenTelemetry.Instrumentation.Digma.Tests/TestTracingDecorator.cs index f433442..fee0461 100644 --- a/src/OpenTelemetry.Instrumentation.Digma.Tests/TestTracingDecorator.cs +++ b/src/OpenTelemetry.Instrumentation.Digma.Tests/TestTracingDecorator.cs @@ -28,6 +28,21 @@ public void Activity_Created_For_Attribute_Marked_Method() "MethodExplicitlyMarkedForTracing", "Action"); }); } + + [TestMethod] + public void Attributes_Injected_To_Marked_Method() + { + DecoratedService service = new DecoratedService(); + IDecoratedService tracingDecorator = TraceDecorator.Create(service); + tracingDecorator.MethodExplicitlyMarkedForTracingWithAttributes(() => + { + Assert.IsNotNull(Activity.Current); + AssertHasCommonTags(Activity.Current, ServiceInterfaceFqn, + "MethodExplicitlyMarkedForTracingWithAttributes", "Action"); + AssertHasTag(Activity.Current, "att1", "value1"); + + }); + } [TestInitialize] public void SetupOtel() @@ -106,4 +121,10 @@ private void AssertHasCommonTags(Activity? activity, new KeyValuePair("code.function.parameter.types", expectedParameterTypes)); } } + + private void AssertHasTag(Activity? activity, string name, string value) + { + var kvpTags = activity.Tags.ToArray(); + CollectionAssert.Contains(kvpTags, new KeyValuePair(name, value)); + } } \ No newline at end of file diff --git a/src/OpenTelemetry.Instrumentation.Digma/Helpers/TracingDecorator.cs b/src/OpenTelemetry.Instrumentation.Digma/Helpers/TracingDecorator.cs index 0f27d2f..8d61451 100644 --- a/src/OpenTelemetry.Instrumentation.Digma/Helpers/TracingDecorator.cs +++ b/src/OpenTelemetry.Instrumentation.Digma/Helpers/TracingDecorator.cs @@ -1,5 +1,7 @@ -using System.Diagnostics; +using System.Collections.Concurrent; +using System.Diagnostics; using System.Reflection; +using Microsoft.Extensions.Caching.Memory; using OpenTelemetry.Instrumentation.Digma.Helpers.Attributes; using OpenTelemetry.Trace; @@ -12,6 +14,10 @@ public class TraceDecorator : DispatchProxy where TDecorated : class private IActivityNamingSchema _namingSchema = new MethodFullNameSchema(); private bool _decorateAllMethods = true; + private readonly ConcurrentDictionary _methodNoActivityAttributeCache = new(); + private readonly ConcurrentDictionary _methodActivitiesTagsCache = new(); + private readonly ConcurrentDictionary _methodActivityAttributeCache = new(); + /// /// Creates a new TraceDecorator instance wrapping the specific object and implementing the TDecorated interface /// @@ -61,31 +67,72 @@ private void SetParameters(TDecorated decorated, IActivityNamingSchema? spanNami protected override object? Invoke(MethodInfo? targetMethod, object?[]? args) { - var noActivityAttribute = targetMethod.GetCustomAttribute(false); - var activityAttribute = targetMethod.GetCustomAttribute(false); - - if (noActivityAttribute == null && (_decorateAllMethods || activityAttribute != null)) + if (targetMethod != null) { - var defaultSpanName = _namingSchema.GetSpanName(_decorated!.GetType(), targetMethod); - using var activity = _activity.StartActivity(activityAttribute?.Name ?? defaultSpanName); - - SpanUtils.AddCommonTags(targetMethod, activity); - InjectAttributes(targetMethod, activity); + + var noActivityAttribute = GetMethodNoActivityAttribute(targetMethod); + var activityAttribute = GetMethodActivityAttribute(targetMethod); - if (activityAttribute?.RecordExceptions == false) + if (noActivityAttribute == null && (_decorateAllMethods || activityAttribute != null)) { - return InvokeDecoratedExecution(targetMethod, args); - } + var defaultSpanName = _namingSchema.GetSpanName(_decorated!.GetType(), targetMethod); + using var activity = _activity.StartActivity(activityAttribute?.Name ?? defaultSpanName); + + SpanUtils.AddCommonTags(targetMethod, activity); + InjectAttributes(targetMethod, activity); + + if (activityAttribute?.RecordExceptions == false) + { + return InvokeDecoratedExecution(targetMethod, args); + } - return WrapWithRecordException(activity, () => InvokeDecoratedExecution(targetMethod, args)); + return WrapWithRecordException(activity, () => InvokeDecoratedExecution(targetMethod, args)); + } } return InvokeDecoratedExecution(targetMethod, args); } + private TAttribute? GetInstanceMethodAttribute(MethodInfo targetMethod) where TAttribute : Attribute + { + return _decorated.GetType().GetMethod(targetMethod.Name)?.GetCustomAttribute(inherit:true); + } + + + private TAttribute? GetFromOrStoreInAttributeCache( + MethodInfo targetMethod, + IDictionary cache) where TAttribute : Attribute + { + TAttribute? attributeInfo; + + if (!cache.TryGetValue(targetMethod.Name, out attributeInfo)) + + { + attributeInfo = GetInstanceMethodAttribute(targetMethod); + cache[targetMethod.Name] = attributeInfo; + + } + + return attributeInfo; + } + private ActivitiesAttributesAttribute? GetActivityTagsForInstanceMethod(MethodInfo targetMethod) + { + return GetFromOrStoreInAttributeCache(targetMethod, _methodActivitiesTagsCache); + } + + private TraceActivityAttribute? GetMethodActivityAttribute(MethodInfo targetMethod) + { + return GetFromOrStoreInAttributeCache(targetMethod, _methodActivityAttributeCache); + } + + private NoActivityAttribute? GetMethodNoActivityAttribute(MethodInfo targetMethod) + { + return GetFromOrStoreInAttributeCache(targetMethod, _methodNoActivityAttributeCache); + } + private void InjectAttributes(MethodInfo targetMethod, Activity? activity) { - var methodActivityAttributes = targetMethod.GetCustomAttribute(inherit: false); + var methodActivityAttributes = GetActivityTagsForInstanceMethod(targetMethod); var classActivityAttributes = _decorated.GetType().GetCustomAttribute(inherit: false); From 19c9e41a2d432e1210e19597df02c09771152be5 Mon Sep 17 00:00:00 2001 From: Roni Dover Date: Wed, 22 Mar 2023 22:23:47 -0700 Subject: [PATCH 2/2] fixed issue where attribute were not taken from class --- .../Stubs/DecoratedService.cs | 4 +- .../TestTracingDecorator.cs | 7 +- .../Helpers/TracingDecorator.cs | 79 ++++++++++++------- 3 files changed, 58 insertions(+), 32 deletions(-) diff --git a/src/OpenTelemetry.Instrumentation.Digma.Tests/Stubs/DecoratedService.cs b/src/OpenTelemetry.Instrumentation.Digma.Tests/Stubs/DecoratedService.cs index b2efd05..53c9d86 100644 --- a/src/OpenTelemetry.Instrumentation.Digma.Tests/Stubs/DecoratedService.cs +++ b/src/OpenTelemetry.Instrumentation.Digma.Tests/Stubs/DecoratedService.cs @@ -55,9 +55,11 @@ public async Task AsyncMethodExplicitlyMarkedForTracing(Action stateValidation) stateValidation(); } - + [TraceActivity()] public async Task AsyncVoid() { + var v = Activity.Current; + await Task.Delay(100); } diff --git a/src/OpenTelemetry.Instrumentation.Digma.Tests/TestTracingDecorator.cs b/src/OpenTelemetry.Instrumentation.Digma.Tests/TestTracingDecorator.cs index 755351b..88af9ed 100644 --- a/src/OpenTelemetry.Instrumentation.Digma.Tests/TestTracingDecorator.cs +++ b/src/OpenTelemetry.Instrumentation.Digma.Tests/TestTracingDecorator.cs @@ -16,7 +16,7 @@ namespace OpenTelemetry.Instrumentation.Digma.Tests; public class TestTracingDecorator { private static readonly string ServiceInterfaceFqn = - "OpenTelemetry.Instrumentation.Digma.Tests.Stubs.IDecoratedService"; + "OpenTelemetry.Instrumentation.Digma.Tests.Stubs.DecoratedService"; [TestMethod] public void Activity_Created_For_Attribute_Marked_Method() @@ -173,8 +173,9 @@ public async Task Activity_Async_Error() // Act #1 try { - await decoratedService.AsyncError(); - Assert.Fail(); + var task = decoratedService.AsyncError(); + await task; + throw task.Exception!; } catch (Exception e) { diff --git a/src/OpenTelemetry.Instrumentation.Digma/Helpers/TracingDecorator.cs b/src/OpenTelemetry.Instrumentation.Digma/Helpers/TracingDecorator.cs index 9a07fce..f505611 100644 --- a/src/OpenTelemetry.Instrumentation.Digma/Helpers/TracingDecorator.cs +++ b/src/OpenTelemetry.Instrumentation.Digma/Helpers/TracingDecorator.cs @@ -17,7 +17,6 @@ public class TraceDecorator : DispatchProxy where TDecorated : class private readonly ConcurrentDictionary _methodNoActivityAttributeCache = new(); private readonly ConcurrentDictionary _methodActivitiesTagsCache = new(); private readonly ConcurrentDictionary _methodActivityAttributeCache = new(); - /// /// Creates a new TraceDecorator instance wrapping the specific object and implementing the TDecorated interface /// @@ -45,39 +44,60 @@ private void SetParameters(TDecorated decorated, IActivityNamingSchema? spanNami } } - private object? InvokeDecoratedExecution(Activity? activity, MethodInfo? targetMethod, object?[]? args, bool? recordException) + private bool IsAsync(MethodInfo targetMethod) + { + var taskType = typeof(Task); + return targetMethod.ReturnType == taskType || + targetMethod.ReturnType.IsSubclassOf(taskType); + } + private object? InvokeAsyncDecoratedExecution(Activity? activity, MethodInfo? targetMethod, object?[]? args, + bool? recordException) { + activity.Start(); + var resultTask = targetMethod.Invoke(_decorated, args) as Task; + + resultTask.ContinueWith(task => + { + if (task.Exception != null && (recordException ?? true)) + { + activity?.RecordException(task.Exception); + } + + activity?.Stop(); + + activity?.Dispose(); + + + },TaskContinuationOptions.AttachedToParent | TaskContinuationOptions.ExecuteSynchronously); + + return resultTask; + } + + private object? InvokeDecoratedExecution(Activity? activity, MethodInfo? targetMethod, object?[]? args, + bool? recordException) + { + object? result; try { - result = targetMethod.Invoke(_decorated, args); + using (activity) + { + result = targetMethod.Invoke(_decorated, args); + } } catch (Exception e) { - if(recordException ?? true) + if (recordException ?? true) activity?.RecordException(e); - - activity?.Dispose(); + throw; } - if (result is Task resultTask) - { - resultTask.ContinueWith(task => - { - if (task.Exception != null && (recordException ?? true)) - { - activity?.RecordException(task.Exception); - } - - activity?.Dispose(); - }, TaskContinuationOptions.AttachedToParent | TaskContinuationOptions.ExecuteSynchronously); - - return result; - } - activity?.Dispose(); return result; + + + } protected override object? Invoke(MethodInfo? targetMethod, object?[]? args) @@ -88,23 +108,26 @@ private void SetParameters(TDecorated decorated, IActivityNamingSchema? spanNami var noActivityAttribute = GetMethodNoActivityAttribute(targetMethod); var activityAttribute = GetMethodActivityAttribute(targetMethod); - if (noActivityAttribute == null && (_decorateAllMethods || activityAttribute != null)) + if (noActivityAttribute == null && _decorateAllMethods ) { var classType = _decorated!.GetType(); var defaultSpanName = _namingSchema.GetSpanName(classType, targetMethod); - using var activity = _activity.StartActivity(activityAttribute?.Name ?? defaultSpanName); + var activity = _activity.StartActivity(activityAttribute?.Name ?? defaultSpanName); SpanUtils.AddCommonTags(classType,targetMethod, activity); InjectAttributes(targetMethod, activity); - if (activityAttribute?.RecordExceptions == false) + if (IsAsync(targetMethod)) { - return InvokeDecoratedExecution(targetMethod, args); + return InvokeAsyncDecoratedExecution(activity, targetMethod, args, + activityAttribute?.RecordExceptions); } - + return InvokeDecoratedExecution(activity, targetMethod, args, activityAttribute?.RecordExceptions); } + + } return InvokeDecoratedExecution(null, targetMethod, args, null); @@ -155,7 +178,7 @@ private void InjectAttributes(MethodInfo targetMethod, Activity? activity) if (methodActivityAttributes != null) { - foreach (var key in classActivityAttributes.Attributes.Keys) + foreach (var key in methodActivityAttributes.Attributes.Keys) { activity.AddTag(key, methodActivityAttributes.Attributes[key]); } @@ -165,7 +188,7 @@ private void InjectAttributes(MethodInfo targetMethod, Activity? activity) { foreach (var key in classActivityAttributes.Attributes.Keys) { - activity.AddTag(key, methodActivityAttributes.Attributes[key]); + activity.AddTag(key, classActivityAttributes.Attributes[key]); } } }