Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
This pull request introduces comprehensive SDK statistics tracking and enhances the notification system to support telemetry retry events. The main objective is to enable periodic reporting of SDK usage metrics (success, dropped, and retry counts) and provide visibility into the complete telemetry lifecycle including retry scenarios.
Changes:
- Adds
eventsRetrynotification callback to INotificationManager and INotificationListener interfaces for tracking telemetry retry events with HTTP status codes - Implements
SdkStatsNotificationCbk- a notification listener that accumulates telemetry counts by type and periodically reports them as metrics - Integrates SDK stats listener into AISku with automatic initialization (enabled by default) and proper cleanup during unload
- Updates Sender to trigger notifications for sent, discarded, and retried events, storing baseType in IInternalStorageItem for telemetry type mapping
- Adds comprehensive unit tests for the SDK stats notification callback covering all major scenarios
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| shared/AppInsightsCore/src/interfaces/ai/INotificationManager.ts | Adds eventsRetry method signature to notification manager interface |
| shared/AppInsightsCore/src/interfaces/ai/INotificationListener.ts | Adds optional eventsRetry callback to listener interface |
| shared/AppInsightsCore/src/index.ts | Exports new SDK stats notification callback factory and interfaces |
| shared/AppInsightsCore/src/core/SdkStatsNotificationCbk.ts | New file implementing the SDK stats accumulator and periodic metric flushing logic |
| shared/AppInsightsCore/src/core/NotificationManager.ts | Implements eventsRetry notification dispatch to all registered listeners |
| shared/AppInsightsCore/src/constants/InternalConstants.ts | Adds STR_EVENTS_RETRY constant for the new notification type |
| shared/AppInsightsCore/Tests/Unit/src/aiunittests.ts | Registers new SDK stats notification callback test suite |
| shared/AppInsightsCore/Tests/Unit/src/ai/SdkStatsNotificationCbk.Tests.ts | New comprehensive test suite covering all SDK stats functionality |
| channels/applicationinsights-channel-js/src/Sender.ts | Adds notification triggers for sent/discarded/retry events and extracts telemetry items for notifications |
| channels/applicationinsights-channel-js/src/Interfaces.ts | Adds bT (baseType) property to IInternalStorageItem for telemetry type mapping |
| AISKU/src/AISku.ts | Integrates SDK stats listener with initialization, configuration, and unload handling |
shared/AppInsightsCore/Tests/Unit/src/ai/SdkStatsNotificationCbk.Tests.ts
Show resolved
Hide resolved
JacksonWeber
changed the title
JacksonWeber
force-pushed
the
jacksonweber/enable-customer-sdk-stats
branch
from
9ed6655 to
7eaafe0
Compare
| // Notify listeners of successful send | ||
| let mgr = _getNotifyMgr(); | ||
| if (mgr) { | ||
| let items = _extractTelemetryItems(payload); |
There was a problem hiding this comment.
Follow up PR -- lets try and minimize the need to call this, it might be a large PR as it will most likely require changing the whole serialization process (like we do in 1ds-post-js) where we convert to a string as late as possible so at this level we are still playing with the objects -- there is a gotya around the persistence to the session storage / array storage today for this though.
| * [Optional] SDK Stats configuration for periodic reporting of telemetry pipeline metrics. | ||
| * @since 3.3.12 | ||
| */ | ||
| sdkStats?: ISdkStatsConfig; |
There was a problem hiding this comment.
I just realized something :-)
There is no "defaults" being applied as part of this PR (that I can see), either in the AppInsightsCore (applies to both AI and 1DS) or the AISKU (just for AI) -- which means the values won't be dynamic as they won't exist during initialization -- we should set the defaults of this config block to the expected defaults (this is in addition to the "feature" enablement.
There was a problem hiding this comment.
We should also have tests (I've not gone through the tests yet -- so ignore if you already have this), that test the enabling / disabling of the feature and changing of the settings within the sdkstats to ensure they are being followed.
The "gotya" with the dynamic config is that the easiest way is to use the fake timers and because the dynamic config changes are "notified" via a setTimeout(..., 0) you need to use the this.clock.tick(1) to simulate this after changing a config value for it to be applied.
There was a problem hiding this comment.
Added defaults + tests to ensure that dynamic config updates change values upon next flush.
| if (!items || !items.length) { | ||
| return; | ||
| } | ||
| if (!_safeKey(code)) { |
There was a problem hiding this comment.
nit: this could be collapsed into the above to return the number of "return" (non-minifiable code) keywords.
Ideally, invert the logic to remove all return usages and just put the code below within an if that validates the pre-conditions
There was a problem hiding this comment.
Collapsed the two early-return guard clauses into a single if check and removed the return keywords.
| } | ||
|
|
||
| // Flush dropped and retry counts via common helper | ||
| _flushBucketed(_droppedCounts, MET_DROPPED, "drop.code"); |
There was a problem hiding this comment.
is the "." part of the specification?
As this is going to be problematic for 1DS as there is a "mode" where this will get expanded into "get: { code: xxx }" when sent. By default this mode is now off, but there are still some users that need this for their backend storage.
There was a problem hiding this comment.
Unfortunately it is. Do we need to discuss changing the spec on this?
| if (objHasOwn(bucket, telType)) { | ||
| var cnt = bucket[telType]; | ||
| if (cnt > 0) { | ||
| var props: { [key: string]: any } = {}; |
There was a problem hiding this comment.
We could (I think -- minification comment) collapse this into the _createMetric function so that we just pass 3 arguments (code and codePropKey are optional) so that we only create objects and populate these values within the helper function (as the usage of .telemetry_type won' be minified.
There was a problem hiding this comment.
Collapsed the props creation into _createMetric so now it takes telType and optional code & codePropKey params instead of a prebuilt object.
No more multiple object creation
This pull request introduces SDK statistics telemetry collection and notification improvements, as well as enhanced event retry notification and error reporting in the Application Insights JavaScript SDK. The main changes add support for dynamically enabling/disabling SDK stats telemetry, improve notification to listeners about retried and discarded events (with status codes), and enhance test coverage for these new behaviors.
SDK Statistics Telemetry and Dynamic Listener Management
SdkStats) in the main configuration and enabled dynamic creation and cleanup of the SDK stats notification listener (_sdkStatsListener) based on feature flags. The listener is properly unloaded and flushed with other telemetry during shutdown. (AISKU/src/AISku.ts) [1] [2] [3] [4] [5] [6] [7]Event Retry and Notification Enhancements
channels/1ds-post-js/src/PostChannel.ts) [1] [2] [3]Testing Improvements
eventsRetrynotification is fired when events are requeued after a retriable failure, ensuring the feature works as intended. (channels/1ds-post-js/test/Unit/src/PostChannelTest.ts) [1] [2] [3]Channel Plugin Notification Improvements
baseTypefor SDK stats mapping.channels/applicationinsights-channel-js/src/Interfaces.ts,channels/applicationinsights-channel-js/src/Sender.ts) [1] [2] [3] [4] [5] [6] [7] [8]