Repository navigation
Fix telemetry component assembly identification - #4490
Conversation
|
|
Co-authored-by: marcschier <11168470+marcschier@users.noreply.github.com>
Co-authored-by: marcschier <11168470+marcschier@users.noreply.github.com>
Co-authored-by: marcschier <11168470+marcschier@users.noreply.github.com>
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
🟡 Changes recommended
Restore legacy null-result fallbacks and address the documentation and missing fallback-test findings.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
This pull request makes telemetry attribution assembly-aware while preserving compatibility with legacy telemetry contexts.
Changes:
- Adds assembly-aware telemetry contracts and caller-assembly routing.
- Updates client, security, and cache metric call sites.
- Adds tests and migration/diagnostic documentation.
File summaries
| File | Summary |
|---|---|
tests/Opc.Ua.Types.Tests/Diagnostics/TelemetryExtensionsTests.cs |
Tests assembly attribution and legacy behavior. |
src/Opc.Ua.Types/Diagnostics/TelemetryExtensions.cs |
Routes assembly-aware telemetry; restore legacy null fallbacks (moderate, 2 votes) and add legacy StartActivity coverage (nit, 1 vote). |
src/Opc.Ua.Types/Diagnostics/TelemetryContextBase.cs |
Implements assembly-aware telemetry factories. |
src/Opc.Ua.Types/Diagnostics/IAssemblyTelemetryContext.cs |
Defines the optional assembly-aware contract. |
src/Opc.Ua.Core/Stack/Client/ClientBase.cs |
Routes client meter creation. |
src/Opc.Ua.Core/Stack/Client/Channels/ClientChannelManager.cs |
Routes optional channel metrics. |
src/Opc.Ua.Core/Security/Crypto/CryptoProviderAuditor.cs |
Routes crypto metrics. |
src/Opc.Ua.Core/Security/Certificates/CertificateManager/CertificateCache.cs |
Routes certificate-cache metrics. |
src/Opc.Ua.Client/NodeCache/NodeCache.cs |
Routes node-cache metrics. |
docs/migrate/2.0.x/telemetry.md |
Documents telemetry migration compatibility. |
docs/Diagnostics.md |
Documents the contract and usage; interface inheritance and explicit extension invocation need correction (nits, 3 and 1 votes). |
Review details
Suppressed comments (4)
docs/Diagnostics.md:79
- Because
ITelemetryContextalready declares an instanceCreateMeter(), the usage exampletelemetry.CreateMeter()later in this document binds to that instance member rather than this extension and still loses caller-assembly attribution. Update the example and guidance to invokeTelemetryExtensions.CreateMeter(telemetry)explicitly, as the stack call sites do.
**Always use the extension methods**. The metric and tracing extensions
capture the component assembly at the call site, and all extensions
provide a default context when the supplied `ITelemetryContext` is
`null`. They use `IAssemblyTelemetryContext` when available and preserve
the fixed sources returned by existing `ITelemetryContext`
src/Opc.Ua.Types/Diagnostics/TelemetryExtensions.cs:136
- The legacy branch drops the existing
?? Default.ActivitySourcefallback. An existingITelemetryContextimplementation that returns null from itsActivitySourceproperty will now return null instead of the default source, contradicting the extension's documented non-null/fallback behavior.
: context.ActivitySource;
src/Opc.Ua.Types/Diagnostics/TelemetryExtensions.cs:163
StartActivityused to obtain the source throughGetActivitySource(), which fell back toDefault.ActivitySourcefor legacy contexts. This direct legacy branch now passes a nullActivitySourcetoStartActivityand can throw when an existing context returns null; preserve the fallback here.
: context.ActivitySource;
src/Opc.Ua.Types/Diagnostics/TelemetryExtensions.cs:164
- The legacy compatibility path in
StartActivityis not exercised by the added tests:LegacyContextUsesExistingSourcesonly checksCreateMeterandGetActivitySource, while this branch now reads the legacyActivitySourcedirectly and starts an activity. Add a listener and assert thatStartActivitysucceeds with the legacy source so a regression in this fallback cannot pass unnoticed.
ITelemetryContext context = telemetry ?? Default;
ActivitySource activitySource = context is IAssemblyTelemetryContext assemblyContext
? assemblyContext.GetActivitySource(assembly)
: context.ActivitySource;
return activitySource.StartActivity(name, kind);
- Files reviewed: 11/11 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
romanett
left a comment
There was a problem hiding this comment.
I´d say adding new Methods taking an assembly to the existing ITelemetryContext is an accepted breaking change in 2.0.
And applying the inling patch.
Then revert the routing through TeleemtryContext extensions
|
@copilot fix ci and address all feedback |
Co-authored-by: marcschier <11168470+marcschier@users.noreply.github.com>
Head branch was pushed to by a user without write access
|
@copilot update the PR Description to reflect the new edits |
Co-authored-by: marcschier <11168470+marcschier@users.noreply.github.com>
Addressed in |
I can’t edit the PR body from this session: the available GitHub API tools are read-only for PR metadata, and the authenticated browser requires unavailable OAuth. The description should replace the optional |
Code coverage✅ Coverage gate passed.
Uncovered changed lines
Coverage is above the recorded baseline - consider ratcheting Thresholds live in |
Codecov Report❌ Patch coverage is ❌ Your patch check has failed because the patch coverage (39.39%) is below the target coverage (80.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## master #4490 +/- ##
==========================================
+ Coverage 81.06% 81.08% +0.01%
==========================================
Files 2056 2056
Lines 303366 303539 +173
Branches 52579 52613 +34
==========================================
+ Hits 245932 246125 +193
+ Misses 39219 39192 -27
- Partials 18215 18222 +7
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Use the existing portable ValueGuards helper in assembly-aware sample and fuzz telemetry overloads. Resolve the server fixture activity source before registering its listener so ShouldListenTo cannot recursively create the same cold source. Preserve source filtering and sampling, add both-branch listener regression coverage, and document safe listener setup.
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Description
Telemetry extensions resolved meters and activity sources as
Opc.Ua.Types, with results dependent on helper inlining. This prevented reliable attribution to the emitting component.CreateMeter(Assembly)andGetActivitySource(Assembly)directly toITelemetryContext. The separateIAssemblyTelemetryContextinterface has been removed.CreateMeter,GetActivitySource, andStartActivityextensions, then forward it to the existing telemetry context.telemetry.CreateMeter(). Use explicitTelemetryExtensions.CreateMeter(telemetry)for nullable-context fallback. Preserve default meter/activity-source fallback when a context returns null.Compatibility: Direct implementations of
ITelemetryContextmust implement the two new assembly-aware members.TelemetryContextBasesupplies both. This interface addition is documented in the 2.0 migration guide; legacy implementer compatibility is not claimed.Local validation
Both original CI failures were reproduced locally before the fixes: a legacy-target compiler error and a stack overflow in the unchanged tracing integration test.
ServerFixtureTelemetryTests.ActivityListenerSamplesOnlyResolvedSource(logging enabled/disabled)TraceableRequestHeaderTest.ReadValuesWithTracingAsyncThe full solution test suite was not run locally. No existing assertions, test selection, coverage thresholds, or CI requirements were weakened.
Related Issues
Checklist
TelemetryExtensions.GetActivitySource**and**CreateMeterboth live in this assembly and read these members, so all 67 call sites insrc/that go through the extensions resolve toOpc.Ua.Types, and the answer additionally depends on whether the extension itself gets inlined — unstable rather than merely wrong. #4483