Azure Functions Instrumentation#1613
Conversation
lucaspimentel
left a comment
There was a problem hiding this comment.
Half-review. Will continue tomorrow 😄
lucaspimentel
left a comment
There was a problem hiding this comment.
Continued from last night. LGTM 👍🏽, my comments are mostly nitpicking.
zacharycmontoya
left a comment
There was a problem hiding this comment.
So far so good 👍🏼
5b3ad5c to
a46e5bb
Compare
b575231 to
5ad7092
Compare
andrewlock
left a comment
There was a problem hiding this comment.
Looks good, mostly minor comments 🙂
|
|
||
| internal Scope Parent { get; } | ||
|
|
||
| internal Scope Root => Parent?.Root ?? this; |
| public static async Task TriggerAllTimer([TimerTrigger(EveryTenSeconds)] TimerInfo myTimer, ILogger log) | ||
| { | ||
| log.LogInformation($"C# Timer trigger function executed at: {DateTime.Now}"); | ||
| await CallFunctionHttp("trigger"); | ||
| } |
There was a problem hiding this comment.
Are we able to run integration tests for the timer/http triggers, as they don't require any Azure infra?
There was a problem hiding this comment.
Yes, I plan to include those in a follow up, but I will try to get them in today.
There was a problem hiding this comment.
I have not successfully run the functions host in our integration test framework yet, but the integration is running on several sample apps and I will monitor them very closely and continue work on the automation.
c66d69a to
e33216a
Compare
| scope = tracer.StartActive(OperationName); | ||
| foreach (var tagProperty in AzureFunctionsTags.AzureFunctionsExtraTags) | ||
| { | ||
| scope.Root.Span.SetTag(tagProperty.Key, tagProperty.Getter(tags)); |
There was a problem hiding this comment.
Does this else happen often, or are these spans usually roots? If this is common, we should consider a custom tags implementation (like AzureFunctionsTags above) to avoid using Span.SetTag() when possible.
There was a problem hiding this comment.
This happens for all HttpTriggers, basically anything that enters the diagnostic observer
There was a problem hiding this comment.
Ok, thanks. It's not necessarily a blocker for this PR, but something to consider (later?) to improve performance.
bcd59bb to
3bb6afe
Compare
0514eb6 to
413c506
Compare
| private static readonly IDatadogLogger Log = DatadogLogging.GetLoggerFor(typeof(AzureFunctionsCommon)); | ||
|
|
||
| public static CallTargetState OnMethodBegin<TTarget>(TTarget instance, IFunctionInstance instanceParam) | ||
| { |
There was a problem hiding this comment.
You can use the generic constraint for IFunctionInstance, that will remove the boxing of the proxy struct. The same for other methods.
| tags = null; | ||
|
|
||
| if (!tracer.Settings.IsIntegrationEnabled(integrationId) || HttpBypassHelper.UriContainsAnyOf(requestUri, tracer.Settings.HttpClientExcludedUrlSubstrings)) | ||
| if (!tracer.Settings.IsIntegrationEnabled(integrationId) || PlatformHelpers.PlatformStrategy.ShouldSkipClientSpan(tracer.ActiveScope) || HttpBypassHelper.UriContainsAnyOf(requestUri, tracer.Settings.HttpClientExcludedUrlSubstrings)) |
There was a problem hiding this comment.
This allows strategic exclusion of http spans.
| SiteKind = "functionapp"; | ||
| SiteType = "function"; | ||
| IsFunctionsApp = true; | ||
| PlatformStrategy.ShouldSkipClientSpan = ShouldSkipClientSpanWithinFunctions; |
There was a problem hiding this comment.
Where the azure function http skip strategy is added
| { | ||
| internal static class PlatformStrategy | ||
| { | ||
| private static Func<Scope, bool> _shouldSkipClientSpan = (s) => false; |
There was a problem hiding this comment.
The default strategy, able to be overridden.
| ?? 20_000; | ||
| #else | ||
| ?? 100; | ||
| ?? 500; |
There was a problem hiding this comment.
I noticed in azure functions that higher load scenarios meant the initial requests could take much longer than 100ms and this resulted in a failure and a retry loop on startup. Increasing to 500ms I noticed no timeouts.
b7b90a1 to
bf40f30
Compare
| Log.Information("IsProfilerAttached: true"); | ||
|
|
||
| var asm = typeof(Instrumentation).Assembly; | ||
| Log.Information($"[Assembly metadata] Location: {asm.Location}"); |
There was a problem hiding this comment.
Should these be debug logs or did you want to keep them as info logs?
There was a problem hiding this comment.
I want these to show up everytime we load Datadog.Trace
They seem invaluable to me for troubleshooting.
Code Coverage Report 📊✔️ Merging #1613 into master will not change line coverage
View the full report for further details: Datadog.Trace Breakdown ✔️
The following classes have significant coverage changes.
The following classes were added in #1613: View the full reports for further details: |
Introduces the Azure Function integration
@DataDog/apm-dotnet