-
Notifications
You must be signed in to change notification settings - Fork 200
Worker instrumentation #3195
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Worker instrumentation #3195
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR updates worker instrumentation to improve telemetry handling by adding support for OpenTelemetry schema version backward compatibility and introducing a new internal activity system for proper context propagation.
- Added support for multiple OpenTelemetry schema versions (1.17.0 and 1.37.0) with backward compatibility
- Replaced the old activity system with a new telemetry provider architecture that creates properly tracked activities
- Updated trace context to include attributes and modified various telemetry-related components
Reviewed Changes
Copilot reviewed 25 out of 25 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/DotNetWorker.Core/FunctionsApplication.cs | Replaced old activity factory with new telemetry provider system |
| src/DotNetWorker.Core/Diagnostics/ | Added new telemetry provider architecture with schema version support |
| src/DotNetWorker.Core/Context/ | Enhanced trace context to include attributes |
| src/DotNetWorker.OpenTelemetry/ | Updated OpenTelemetry package versions and configuration |
| test/ | Updated tests to work with new telemetry system and validate schema versions |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
src/DotNetWorker.Core/Diagnostics/Telemetry/TelemetryProviderV1_37_0.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR has a lot of changes that impact the telemetry from DotNetWorker.Core irrespective of customer using OTel or not. These seem like breaking changes for those customers.
src/DotNetWorker.Core/Diagnostics/Telemetry/TelemetryProviderBase.cs
Outdated
Show resolved
Hide resolved
src/DotNetWorker.Core/Diagnostics/Telemetry/TelemetryProviderV1_17_0.cs
Outdated
Show resolved
Hide resolved
|
|
||
| using var logScope = _logger.BeginScope(scope); | ||
| using Activity? invokeActivity = _functionActivitySourceFactory.StartInvoke(context); | ||
| using var logScope = _logger.BeginScope(_functionTelemetryProvider.GetTelemetryAttributes(context).ToList()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a significant change to log scope. Also what happens if OTel is not enabled here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’ve kept all existing attributes intact and added new ones according to v17.0 without any breaking changes.
If OpenTelemetry/AppInsights isn’t enabled, the activity will be null, but you’ll always have the scope.
The default specification version is v37.0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Won't this new scope approach be adding OTel specific attributes even when customer is using legacy app insights SDK?
I also wonder if we need all of those in the log scope specifically? Or can they be part of the OTel resource?
| internal const string RegionNameEnvVar = "REGION_NAME"; | ||
| internal const string ResourceGroupEnvVar = "WEBSITE_RESOURCE_GROUP"; | ||
| internal const string OwnerNameEnvVar = "WEBSITE_OWNER_NAME"; | ||
| internal const string WorkerSchemaVersion = "1.37.0"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This directly ties to OTel schema version to the package. This will prevent us from servicing old schema versions in new releases. Is that acceptable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’ll add support for new versions as they’re released. Version 1.37 will remain our baseline, and we’ll bump it to a newer base in the next major release.
| /// Gets the attributes associated with the trace. | ||
| /// </summary> | ||
| public abstract IReadOnlyDictionary<string, string> Attributes { get; } | ||
| public virtual IReadOnlyDictionary<string, string> Attributes => new Dictionary<string, string>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
consider using a common empty dictionary to avoid any allocations when this is not overridden.
Could even use ImmutableDictionary<string, string>.Empty
| { | ||
| OpenTelemetrySchemaVersion.V1_17_0 => new TelemetryProviderV1_17_0(), | ||
| OpenTelemetrySchemaVersion.V1_37_0 => new TelemetryProviderV1_37_0(), | ||
| _ => throw new InvalidOperationException($"Unsupported OpenTelemetry schema version: {version}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider ArgumentOutOfRangeException or ArgumentException
| Activator.CreateInstance(startupCodeExecutorInfoAttr.StartupCodeExecutorType) as WorkerExtensionStartup; | ||
| startupCodeExecutorInstance!.Configure(builder); | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit remove extra whitespace
| } | |
| } |
| => OpenTelemetrySchemaVersion.V1_37_0; | ||
|
|
||
| protected override ActivityKind Kind | ||
| => ActivityKind.Internal; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we assuming the func host emits the Server span?
|
|
||
| using var logScope = _logger.BeginScope(scope); | ||
| using Activity? invokeActivity = _functionActivitySourceFactory.StartInvoke(context); | ||
| using var logScope = _logger.BeginScope(_functionTelemetryProvider.GetTelemetryAttributes(context).ToList()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Won't this new scope approach be adding OTel specific attributes even when customer is using legacy app insights SDK?
I also wonder if we need all of those in the log scope specifically? Or can they be part of the OTel resource?
| internal const string CloudPlatform = "cloud.platform"; | ||
| internal const string CloudRegion = "cloud.region"; | ||
| internal const string CloudResourceId = "cloud.resource.id"; | ||
| internal const string CloudResourceId = "cloud.resource_id"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this not a schema breaking change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question. Wasn't always the cloud resource id defined as cloud.resource_id? Tried to see if there was a resource change from cloud.resource.id to cloud.resource_id but didn't find that in the schema specs.
Issue describing the changes in this PR
resolves #3198
resolves #3199
Pull request checklist
release_notes.md