[Monitor OpenTelemetry] Auto Detect App ID from Connection String if Not Set #36901
Conversation
JacksonWeber
requested review from
MSNev,
hectorhdzg,
jeremydvoss,
lzchen and
rads-1996
as code owners
JacksonWeber
changed the title
JacksonWeber
changed the title
There was a problem hiding this comment.
Pull request overview
This pull request enhances the Azure Monitor OpenTelemetry SDK by automatically populating the microsoft.applicationid resource attribute from the Application Insights connection string when it is not already set. The implementation adds a new utility method to extract the application ID from the connection string and ensures it's applied during resource initialization and updates.
Key changes:
- Adds automatic detection and population of
microsoft.applicationidresource attribute from connection strings - Updates the
ConnectionStringKeytype to includeapplicationidfor type safety - Adds comprehensive unit tests to verify the new behavior
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| sdk/monitor/monitor-opentelemetry/src/shared/config.ts | Implements the _ensureApplicationIdResourceAttribute() method and calls it during initialization and resource updates to populate the resource attribute from connection strings |
| sdk/monitor/monitor-opentelemetry/src/utils/types.ts | Adds applicationid to the ConnectionStringKey type to support parsing this field from connection strings |
| sdk/monitor/monitor-opentelemetry/test/internal/unit/shared/config.test.ts | Adds unit tests to verify the attribute is set from connection strings and not overwritten when already present |
| sdk/monitor/monitor-opentelemetry/CHANGELOG.md | Documents the new feature in the changelog under Features Added |
|
|
||
| public set resource(resource: Resource) { | ||
| this._resource = this._resource.merge(resource); | ||
| this._ensureApplicationIdResourceAttribute(); |
There was a problem hiding this comment.
Calling _ensureApplicationIdResourceAttribute() in the resource setter creates a potential issue. When a resource is set or merged through this setter, it will always attempt to add the microsoft.applicationid from the connection string, even if the resource being set intentionally doesn't have this attribute or has a different value. This could lead to unexpected behavior where setting a resource causes side effects beyond the merge operation. Consider removing this call from the setter and only calling it at strategic points in the constructor after all configuration sources have been merged. The setter should remain a simple merge operation without additional side effects.
| it("does not overwrite existing microsoft.applicationid on resource", () => { | ||
| const customResource = resourceFromAttributes({ | ||
| "microsoft.applicationid": "custom-app-id", | ||
| }); | ||
|
|
||
| const config = new InternalConfig({ | ||
| resource: customResource, | ||
| azureMonitorExporterOptions: { | ||
| connectionString: | ||
| "InstrumentationKey=1aa11111-bbbb-1ccc-8ddd-eeeeffff3333;ApplicationId=from-conn", | ||
| }, | ||
| }); | ||
|
|
||
| assert.strictEqual(config.resource.attributes["microsoft.applicationid"], "custom-app-id"); | ||
| }); |
There was a problem hiding this comment.
Consider adding test cases for additional scenarios: (1) connection string provided via azureMonitorExporterOptions without pre-existing microsoft.applicationid in the resource, (2) connection string that lacks an ApplicationId field to verify graceful handling, (3) empty or undefined connection string to ensure no errors occur. These tests would improve coverage and confidence in the implementation's robustness across different configuration scenarios.
| const connectionString = | ||
| this.azureMonitorExporterOptions?.connectionString || | ||
| process.env["APPLICATIONINSIGHTS_CONNECTION_STRING"]; | ||
| const parsed = ConnectionStringParser.parse(connectionString); |
There was a problem hiding this comment.
Parsing should be already there? why running this again?
There was a problem hiding this comment.
Looks like we don't use it anywhere else in this file, but if we really wanted to avoid calling this parse method more than once in the SDK we could avoid doing so in live metrics and the browser SDK loader by refactoring the parse method to save a copy of the parsed connection string to a publicly accessible variable on the ConnectionStringParser.
|
|
||
| if (applicationId) { | ||
| this._resource = this._resource.merge( | ||
| resourceFromAttributes({ "microsoft.applicationid": applicationId }), |
There was a problem hiding this comment.
microsoft.applicationid will end as a custom property, is this the expectation?
There was a problem hiding this comment.
It'll end up as a custom property on the resource which should be fine. Let me create a spec PR to ensure that we can agree on this though.
There was a problem hiding this comment.
Updated the value to be applicationId and confirmed Alex wants this to report the applicationId value outside of VM envs as well. https://github.com/aep-health-and-standards/Telemetry-Collection-Spec/pull/730
3 participants
Packages impacted by this PR
@azure/monitor-opentelemetry
Describe the problem that is addressed by this PR
This pull request enhances the Azure Monitor OpenTelemetry SDK by ensuring that the
microsoft.applicationidresource attribute is automatically populated from the connection string if it is not already set. It introduces a new utility to parse the connection string for this attribute and adds corresponding unit tests to verify the behavior. The changes improve configuration reliability and make it easier to ensure correct resource attributes are set.Resource attribute population:
InternalConfigto automatically set themicrosoft.applicationidresource attribute from the connection string if it is missing, using a new_ensureApplicationIdResourceAttributemethod. This is invoked during initialization and when the resource is set. [1] [2] [3]resourceFromAttributesandConnectionStringParserto support the new attribute population logic. [1] [2]Type and utility updates:
ConnectionStringKeytype to includeapplicationidfor better type safety when parsing connection strings.Testing improvements:
microsoft.applicationidis set from the connection string when missing and is not overwritten if already present in the resource.Checklists