-
Notifications
You must be signed in to change notification settings - Fork 0
feat: Add exception tracking, scoped context, and exception severity to telemetry (OTel-ready contract) #23
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?
Conversation
…elemetry - Add TelemetrySeverity enum with Critical, Error, Warning, Info, Debug levels - Add CreateScope method to ITelemetry for nested/merged scoped context - Add TrackException method to ITelemetry with severity support - Implement exception tracking in Telemetry class (Application Insights backend) - Implement exception tracking in FileTelemetry class with structured JSON - Implement scoped context in both Telemetry and FileTelemetry classes - Update TelemetryAdapter and TelemetryGenericFactory to delegate new methods - Scope properties/measurements override parent values on key conflicts - All telemetry operations maintain no-throw guarantees Co-authored-by: carldebilly <[email protected]>
|
|
- Add ExceptionTelemetryTests with 10 test cases covering: - Basic exception tracking - Exception with properties and measurements - Severity level mapping (Critical, Error, Warning, Info, Debug) - Default severity behavior - Null safety checks - Stack trace inclusion - Add ScopedTelemetryTests with 13 test cases covering: - Scope property and measurement inheritance - Nested scope merging - Child scope overrides parent on key conflicts - Event properties override scope properties - Scope applies to both events and exceptions - Empty scope handling - Multiple events in same scope - Deeply nested scopes All 33 tests pass (14 existing + 19 new) Co-authored-by: carldebilly <[email protected]>
- Extract common logic from overloaded GetEventProperties/GetEventMeasures methods in Telemetry class - Extract common logic from overloaded MergeProperties/MergeMeasurements methods in FileTelemetry class - Use IEnumerable<KeyValuePair<T>> as common base for both IDictionary and IReadOnlyDictionary - Maintain public overloads for type safety while reducing duplication - All 33 tests still pass Co-authored-by: carldebilly <[email protected]>
- Add TelemetryTypeEvent and TelemetryTypeException constants - Replace string literals with constants to prevent typos - Ensures consistency across telemetry entry types Co-authored-by: carldebilly <[email protected]>
…nostic wrapper - Remove CreateScope from ITelemetry interface (now extension method) - Add ScopedTelemetry wrapper class for implementation-agnostic scope support - Add TelemetryExtensions with CreateScope extension method - Remove scope-related fields and logic from Telemetry and FileTelemetry classes - Scope merging now handled by ScopedTelemetry wrapper instead of each implementation - Eliminates code duplication and fragile object initializer pattern - All 33 tests still pass Co-authored-by: carldebilly <[email protected]>
- Remove unused MergeScopeProperties and MergeScopeMeasurements methods - Consolidate MergeProperties/MergeMeasurements overloads using Core methods - Fix bug in MergePropertiesCore (was creating Dictionary<double> instead of Dictionary<string>) - Eliminate code duplication in merge logic Co-authored-by: carldebilly <[email protected]>
carldebilly
left a comment
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.
@copilot Some reviews...
- Fix ScopedTelemetry.Dispose() to not dispose inner telemetry (scope wrapper shouldn't dispose shared instance) - Rename TelemetrySeverity to ExceptionSeverity to reflect it's only used for exceptions - Add comments explaining Thread.Yield() behavior on single-threaded WASM (it's a no-op, safe to use) - All 33 tests still pass Co-authored-by: carldebilly <[email protected]>
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 adds exception tracking, scoped context, and severity levels to the Uno.DevTools.Telemetry library, creating an OpenTelemetry-ready contract without implementing OTel backends yet.
Key Changes
- Introduces
TrackExceptionmethod toITelemetryinterface with anExceptionSeverityenum (Critical, Error, Warning, Info, Debug) - Adds
CreateScopeextension method that returns aScopedTelemetrywrapper for nested, merged contextual properties and measurements - Implements exception tracking in both Application Insights backend (with severity mapping to
SeverityLevel) and file-based telemetry (as structured JSON)
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Uno.DevTools.Telemetry/ExceptionSeverity.cs | New enum defining five severity levels for exception telemetry |
| src/Uno.DevTools.Telemetry/ITelemetry.cs | Adds TrackException method to core interface |
| src/Uno.DevTools.Telemetry/TelemetryExtensions.cs | New extension method CreateScope providing default scope implementation |
| src/Uno.DevTools.Telemetry/ScopedTelemetry.cs | Internal wrapper class that merges scope properties/measurements with event-specific data |
| src/Uno.DevTools.Telemetry/Telemetry.cs | Application Insights implementation with severity mapping and lock-free exception tracking |
| src/Uno.DevTools.Telemetry/FileTelemetry.cs | File-based implementation serializing exceptions as structured JSON |
| src/Uno.DevTools.Telemetry/TelemetryAdapter.cs | Updated adapter to forward TrackException calls |
| src/Uno.DevTools.Telemetry/TelemetryServiceCollectionExtensions.cs | Updated DI factory to implement TrackException |
| src/Uno.DevTools.Telemetry.Tests/ExceptionTelemetryTests.cs | 11 new tests covering exception tracking scenarios |
| src/Uno.DevTools.Telemetry.Tests/ScopedTelemetryTests.cs | 10 new tests covering scope nesting, merging, and override behavior |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| private IDictionary<string, string>? GetEventProperties(IReadOnlyDictionary<string, string>? properties) | ||
| { | ||
| if (properties == null) | ||
| { | ||
| return _commonProperties is IDictionary<string, string> commonProperties | ||
| ? commonProperties | ||
| : new Dictionary<string, string>(_commonProperties?.ToDictionary(kvp => kvp.Key, kvp => kvp.Value) | ||
| ?? new Dictionary<string, string>(0)); | ||
| } | ||
|
|
||
| var eventProperties = new Dictionary<string, string>(_commonProperties?.ToDictionary(kvp => kvp.Key, kvp => kvp.Value) | ||
| ?? new Dictionary<string, string>(0)); | ||
| foreach (var property in properties) | ||
| { | ||
| eventProperties[property.Key] = property.Value; | ||
| } | ||
| return eventProperties; | ||
| } |
Copilot
AI
Jan 9, 2026
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 duplicate implementation of GetEventProperties that differs only in parameter type (IReadOnlyDictionary vs IDictionary). Consider consolidating both overloads to call a shared private helper method that accepts IEnumerable<KeyValuePair<string, string>>? to eliminate the code duplication.
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.
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.
Consolidated in commit d80c145. Both overloads now delegate to shared GetEventPropertiesCore and GetEventMeasuresCore helper methods that accept IEnumerable<KeyValuePair<T>>, eliminating the duplication.
| private IDictionary<string, double> GetEventMeasures(IReadOnlyDictionary<string, double>? measurements) | ||
| { | ||
| var eventMeasurements = new Dictionary<string, double>(_commonMeasurements?.ToDictionary(kvp => kvp.Key, kvp => kvp.Value) | ||
| ?? new Dictionary<string, double>(0)); | ||
| if (measurements != null) | ||
| { | ||
| foreach (var measurement in measurements) | ||
| { | ||
| eventMeasurements[measurement.Key] = measurement.Value; | ||
| } | ||
| } | ||
| return eventMeasurements; | ||
| } |
Copilot
AI
Jan 9, 2026
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 duplicate implementation of GetEventMeasures that differs only in parameter type (IReadOnlyDictionary vs IDictionary). Consider consolidating both overloads to call a shared private helper method that accepts IEnumerable<KeyValuePair<string, double>>? to eliminate the code duplication.
- Extract common logic into GetEventPropertiesCore and GetEventMeasuresCore helper methods - Overloads for IDictionary and IReadOnlyDictionary now delegate to core methods accepting IEnumerable - Eliminates code duplication while maintaining type-safe public overloads - All 33 tests still pass Co-authored-by: carldebilly <[email protected]>
GitHub Issue (If applicable): #
PR Type
What is the current behavior?
ITelemetryonly tracks events with string properties and double measurements. No support for exception telemetry, contextual scoping, or severity levels.What is the new behavior?
Breaking API Changes
Added exception tracking and exception severity enum to
ITelemetry, plusCreateScopeas an extension method:Architecture
Scope Implementation:
CreateScopeis an extension method (inTelemetryExtensions) that works with anyITelemetryimplementationScopedTelemetrywrapper - an implementation-agnostic class that handles scope mergingScope Semantics:
TrackEventandTrackExceptionScopedTelemetrywrapperBackend Implementations
Application Insights (
Telemetry):ExceptionSeverity→SeverityLevelTelemetryClient.TrackException(ExceptionTelemetry)File-based (
FileTelemetry):{ "Type": "exception", "Timestamp": "...", "Severity": "Error", "Exception": { "Type": "System.InvalidOperationException", "Message": "...", "StackTrace": "..." }, "Properties": { "scopeKey": "scopeValue" }, "Measurements": { "duration": 123.45 } }Example Usage
Code Quality
All duplicate code has been eliminated through refactoring:
GetEventPropertiesandGetEventMeasuresoverloads consolidated using shared Core helper methodsIDictionaryandIReadOnlyDictionaryoverloads delegate toGetEventPropertiesCoreandGetEventMeasuresCorethat acceptIEnumerable<KeyValuePair<T>>ScopedTelemetryfor merge operationsOTel-Ready
Contract carries structured exception data, severity, and scoped context sufficient for future OpenTelemetry backend without API changes.
PR Checklist
Breaking Changes: This PR introduces breaking changes to
ITelemetryinterface - implementations must addTrackExceptionmethod. TheExceptionSeverityenum is new.CreateScopeis provided as an extension method, so no implementation changes required for scope support.Other information
Testing: 33 tests pass (14 existing + 19 new). New tests cover exception tracking, severity mapping, nested scope merging, and override behavior.
Security: CodeQL analysis shows 0 vulnerabilities. All telemetry operations maintain no-throw guarantees.
Architecture: Scope implementation uses extension method + wrapper pattern per code review feedback, eliminating duplication and avoiding fragile patterns in concrete implementations. The scope wrapper correctly does not dispose the inner telemetry instance, which may be shared across multiple scopes or used directly. All helper methods follow the Core pattern to consolidate duplicate implementations and reduce code duplication.
Platform Compatibility: Thread-safe lock-free chaining pattern (using
Interlocked.CompareExchangewithThread.Yield) works correctly on all platforms including single-threaded WASM, where concurrent access doesn't occur and the exchange succeeds immediately.Internal Issue (If applicable):
Original prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.