Enable SDK Stats and route to the SDK Stats ingestion endpoint#2741
Enable SDK Stats and route to the SDK Stats ingestion endpoint#2741JacksonWeber wants to merge 26 commits into
Conversation
Completes and enables the SDK Stats manager (createStatsMgr) and routes the resulting events to the distro-owned SDK Stats ingestion endpoint (stats.monitor.azure.com / eu.stats.monitor.azure.com) instead of the customer's breeze endpoint, matching the Microsoft OpenTelemetry distro. - StatsBeat.ts: add SDK Stats endpoint constants, EU/non-EU region detection, per-event destination + placeholder iKey stamping, enabled-by-default feature gate, and createSdkStatsMgrConfig(). - AppInsightsCore / IAppInsightsCore / index: restore getStatsBeat, setStatsMgr, fields, unload cleanup, stubs, and exports. - Sender.ts: restore request-counting hooks and statsBeatData; redirect SDK Stats items to the SDK Stats endpoint via a per-item URL override, bypassing the customer buffer; exclude SDK Stats sends from counting. - AISku.ts: create, init and set the SDK Stats manager after core init. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Adds support for sending SDK Stats to either the new SDK Stats endpoint (stats.monitor.azure.com) or the legacy breeze endpoints, selectable at runtime via config.stats (IStatsBeatConfig), which is overridable through the CDN / dynamic config. - StatsType.ts: add eStatsEndpointType (SdkStats / Breeze) enum. - IStatsBeat.ts: add IStatsBeatConfig.mode and IStatsBeatKeyMap.url. - IConfiguration.ts: add stats?: IStatsBeatConfig (dynamic-config surface). - StatsBeat.ts: factor EU detection into _isEuEndpoint; add getStatsBreezeIKey + breeze SDK Stats iKey constants; resolve the destination iKey/endpoint per-event in _track based on the (dynamic) mode; createSdkStatsMgrConfig now reads config.stats so the endpoint and key map can be overridden via the CDN at runtime. Defaults to the SDK Stats endpoint. - index.ts: export the new enum, helper and constants. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Cache telemetryItem.data reference to avoid repeated property lookups. Reduces minified output size in hot path (runs for every telemetry item). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Uncommented all SDK Stats test files - Updated test case names from 'StatsBeat' to 'SDK Stats' - Updated test assertions to use 'SDK Stats' terminology - Fixed import paths for relocated modules - Tests now reference the feature as 'SDK Stats' in all user-facing messages Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR re-enables the SDK Stats (StatsBeat) manager in the Application Insights JavaScript SDK and adds support for routing SDK Stats telemetry to a distro-owned ingestion endpoint (EU vs non-EU), rather than piggy-backing on the customer’s Breeze endpoint. It introduces a runtime-selectable destination mode via config/dynamic config, restores core APIs for stats manager access, and updates the Sender channel to redirect per-item sends when the SDK Stats endpoint override marker is present.
Changes:
- Added SDK Stats endpoint + iKey resolution (EU vs non-EU), plus configurable routing mode (SDK Stats endpoint vs legacy Breeze) and per-event destination stamping.
- Restored core APIs (
getStatsBeat/setStatsMgr) and exports to support enabling SDK Stats from AISKU without increasing core bundle size. - Updated Sender to detect SDK Stats endpoint overrides, bypass the customer buffer for SDK Stats sends, and re-enabled associated unit tests.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| shared/AppInsightsCore/Tests/Unit/src/aiunittests.ts | Re-enabled SDK Stats unit test suite execution. |
| shared/AppInsightsCore/Tests/Unit/src/ai/StatsBeat.Tests.ts | Re-enabled and updated core SDK Stats unit tests. |
| shared/AppInsightsCore/src/interfaces/ai/IStatsBeat.ts | Added SDK Stats routing config surface (mode, per-keymap url). |
| shared/AppInsightsCore/src/interfaces/ai/IConfiguration.ts | Added stats?: IStatsBeatConfig for runtime/dynamic config control. |
| shared/AppInsightsCore/src/interfaces/ai/IAppInsightsCore.ts | Restored getStatsBeat / setStatsMgr APIs on the core interface. |
| shared/AppInsightsCore/src/index.ts | Re-exported SDK Stats enums, interfaces, and core helper APIs/constants. |
| shared/AppInsightsCore/src/enums/ai/StatsType.ts | Added eStatsEndpointType / StatsEndpointType for routing mode selection. |
| shared/AppInsightsCore/src/core/StatsBeat.ts | Implemented SDK Stats endpoint/iKey resolution, routing mode support, and default manager config helper. |
| shared/AppInsightsCore/src/core/AppInsightsCore.ts | Restored core state + lifecycle cleanup for stats manager / stats beat instance. |
| channels/applicationinsights-channel-js/Tests/Unit/src/StatsBeat.tests.ts | Re-enabled channel-level SDK Stats tests for Sender request counting / emission. |
| channels/applicationinsights-channel-js/Tests/Unit/src/aichannel.tests.ts | Re-enabled channel SDK Stats tests in the channel test runner. |
| channels/applicationinsights-channel-js/src/Sender.ts | Added per-item SDK Stats endpoint override handling + direct send path, and restored request counting hooks. |
| AISKU/src/AISku.ts | Enabled SDK Stats from AISKU after core init via createSdkStatsMgrConfig(). |
- Initialize the core before init-ing the stats manager against the same core instance so the manager actually enables (channel + core tests) - Match the stats endCfg to the Sender's endpoint so metrics are tracked - Pass an IStatsBeatState to getStatsBeat() when wiring the count spy - Fix the xhr test config built with '&&' that discarded the Sender config - Guard the dynamic-config test against double core initialization Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
IConfiguration exposes 'stats' (IStatsBeatConfig) at the top level, not under a '_sdk' wrapper. Update the SDK Stats unit tests to set config.stats and read cfg.stats so they compile against IConfiguration. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…size budget - Tick the real short interval (shrtInt*1000 ms) so the stats timer fires in the count/countException tests - FeatureOptInMode.none and a null mode fall back to the SDK default (enabled), so assert SDK Stats stays enabled rather than being removed - Bump core size budget to 135 KB raw / 55 KB deflate to cover the re-enabled getStatsBeat/setStatsMgr core APIs (was 133/54) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The 'SDK Stats initializes when stats is true' test initialized the stats manager but never called core.setStatsMgr(), so core.getStatsBeat() returned null. Register the manager with the core so the instance is created. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Enabling SDK Stats from AISKU pulls in the re-enabled core getStatsBeat/setStatsMgr APIs and stats manager wiring, growing the AISKU bundle. Bump the budget to 178 KB / 72 KB deflate (was 175/71). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…-sdk-stats # Conflicts: # AISKU/Tests/Unit/src/AISKUSize.Tests.ts # AISKU/src/AISku.ts # channels/applicationinsights-channel-js/src/Sender.ts # shared/AppInsightsCore/Tests/Unit/src/ai/AppInsightsCoreSize.Tests.ts # shared/AppInsightsCore/src/index.ts
The new SDK Stats code in AppInsightsCore pushes the core bundle past its 135 KB budget and the AISKU bundle past its 178 KB / 72 KB budgets. Bump core to 137 KB and AISKU to 181 KB / 73 KB to match measured sizes. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…g support Replace the detached config snapshot in createSdkStatsMgrConfig with the setDf/ref pattern so SDK Stats defaults live in the single global config (overridable via CDN/dynamic config and the SKUs) and runtime changes are tracked. Updates IStatsMgrConfig.getCfg to receive IWatchDetails. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…g refactor The setDf/ref refactor in createSdkStatsMgrConfig pushes the core deflate size just past 55 KB (55.01 KB) and the AISKU deflate budget; bump core deflate 55 -> 56 KB and AISKU deflate 73 -> 74 KB to match. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…g from the global config The SDK Stats manager now references the single global config (config.stats) directly and picks up dynamic/CDN changes at runtime, gated behind the sdkStats feature flag (enabled by default, opt-out via featureOptIn). Removes the createSdkStatsMgrConfig factory and IStatsMgrConfig interface; init is now init(core, featureName?). Updates AISKU and StatsBeat tests accordingly. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
| import { INetworkStatsbeat } from "../interfaces/ai/INetworkStatsbeat"; | ||
| import { IStatsBeat, IStatsBeatConfig, IStatsBeatState, IStatsEndpointConfig } from "../interfaces/ai/IStatsBeat"; | ||
| import { IStatsMgr, IStatsMgrConfig } from "../interfaces/ai/IStatsMgr"; | ||
| import { IStatsBeat, IStatsBeatConfig, IStatsBeatKeyMap, IStatsBeatState, IStatsEndpointConfig } from "../interfaces/ai/IStatsBeat"; |
There was a problem hiding this comment.
We should rename all of the "StatsBeat" to SdkStats something that represents that this is the internal SdkStats (as we already have the customer SdkStats using sdkStats...
There was a problem hiding this comment.
Changed to InternalSdkStats. Doesn't look like it impacted bundle sizes negatively, but let me know what you think.
Instead of embedding the ingestion URL, fetch the runtime SDK Stats config (data.stats.monitor.azure.com / eu-data.stats.monitor.azure.com cfg/v1.json), check the enabled flag, and route events to the returned url + /v2/track when enabled. Adds the OneCollector path constant as scaffolding for future 1DS. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Eliminate the legacy 'StatsBeat' name in favor of 'Internal SDK Stats' to distinguish it from the customer-facing SdkStats feature. Interfaces, types, enum members, locals and private functions rename freely (erased or mangled); the two surviving property names were shortened (getStatsBeat -> getSdkStats, statsBeatData -> statsData), so the minified bundle size is unchanged. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…tination Remove the breeze-endpoint mode and key-map/endCfg routing so SDK Stats collection is gated solely on the remote cfg enabled flag and sent to the cfg-provided host. Parsing is fail-closed (enabled === true, url must be a string). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…d config + ref() Address PR review: drop the persistent _internalSdkStatsConfig field and the details.ref() hack. Copy the override fetch fn and interval into local vars in the config-change handler, making the override fetch fn a dynamic leaf so merged runtime updates still re-run the handler. Seed the stats default via a plain object (not cfgDfMerge) so setDf no longer marks stats as a reference. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Address PR review: the setDf cast was only needed because the generic CfgType propagated into onConfigChange. Pin onConfigChange to IConfiguration (we only touch IConfiguration-level config) so setDf infers IConfiguration and the cast is no longer required. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The type field was never read for logic (getSdkStats isolates instances by endpoint, not type) and customer stats are handled separately via createSdkStatsNotifCbk, so the enum is no longer needed. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…zipped) Refactor InternalSdkStats and SdkStatsNotificationCbk to shrink the core minified bundle: objForEachKey merge, shared count/inc helpers, hoisted repeated string literals, arrIndexOf for EU lookup, reuse MetricDataType. Core min.js: raw 139595 -> 139218 (-377 B), gzip 56693 -> 56566 (-127 B). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
testCleanup nulled the initialized AppInsightsCore without unloading it, leaking hooks/timers/config watchers into subsequent tests. Call core.unload(false) when initialized, matching the core test convention. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Revert the _incCounts extraction so success/bucket counts assign directly to their objCreate(null) counters. The extracted target parameter hid the null-prototype provenance, causing CodeQL js/prototype-polluting-assignment to flag target[t]. Direct assignment restores the safe-sink recognition. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
| } | ||
|
|
||
| /** Default SDK Stats config fetch (fetch, else XHR); calls oncomplete with the parsed config or null. */ | ||
| function _defaultStatsCfgFetch(cfgUrl: string, oncomplete: (result: IInternalSdkStatsCfgResult) => void): void { |
| if (isArray(payload) && payload.length > 0) { | ||
| let batch = _self._buffer.batchPayloads(payload); | ||
| let headers = _getHeaders(); | ||
| let payloadData: IInternalPayloadData = { | ||
| data: batch, | ||
| urlString: _endpointUrl, | ||
| urlString: urlOverride || _endpointUrl, | ||
| headers: headers, | ||
| disableXhrSync: _disableXhr, |
| timeout: 0, | ||
| disableXhrSync: false, | ||
| statsData: { | ||
| startTime: "2023-10-01T00:00:00Z" // Simulated start time |
| * gate collection and resolve the ingestion host. EU endpoint is used for EU data-boundary regions. | ||
| */ | ||
| export const STATS_SDK_CFG_URL_NON_EU = "https://data.stats.monitor.azure.com/cfg/v1.json"; | ||
| export const STATS_SDK_CFG_URL_EU = "https://eu-data.stats.monitor.azure.com/cfg/v1.json"; |
There was a problem hiding this comment.
We should minimize the duplication here as strings can't be compressed.
We should have a
const scheme = "https://";
const baseUrl = "data.stats.monitor.azure.com/cfg/v1.json";
and then create the above from these :-)
| * The placeholder instrumentation key used when reporting SDK statistics to the distro-owned | ||
| * SDK Stats ingestion endpoint. The endpoint does not require authentication, the placeholder | ||
| * key only satisfies the connection-string / envelope iKey requirement and is ignored | ||
| * server-side. This matches the convention used by the Microsoft OpenTelemetry distros. |
There was a problem hiding this comment.
"ignored server-side" is not always true.
If the config returns breeze as the URL then no stats will be collected, so we should not send any telemetry (regardless of the backend) without a instrumentation key -- this can be in the config and we can have dynamic config provide / replace the value.
| import { getResponseText, isFeatureEnabled, openXhr } from "../utils/HelperFuncs"; | ||
|
|
||
| const STATS_COLLECTION_SHORT_INTERVAL: number = 900000; // 15 minutes | ||
| const STATS_MIN_INTERVAL_SECONDS = 60; // 1 minute |
There was a problem hiding this comment.
Do we really want to send the events every minute (assuming the SDK is actively sending data)?
I believe that .Net only sends every 30/60 minutes...
Which means we would need to cache this in session / local storage or something.
There was a problem hiding this comment.
Let's discuss this further, my understanding was that we can't expect the session to live that long so lowering the timing threshold would give us a better chance of collecting telemetry. Let's discuss further in the meeting Monday.
| export const STATS_SDK_FEATURE = "sdkStats"; | ||
|
|
||
| // EU data-boundary regions, mirrors the EU region set used by the Azure Monitor OpenTelemetry exporter | ||
| const STATS_EU_REGIONS = [ |
There was a problem hiding this comment.
We can probably reduce this eg.
"france" vs "francecentral", "francesouth" etc.
or we extract the duplicate portions "south", "central", "east", "west" and the countries and then create the merge strings within the array.
| * Returns the SDK Stats config URL (`cfg/v1.json`) for the endpoint (EU vs non-EU). | ||
| */ | ||
| export function getStatsCfgUrl(endpoint: string): string { | ||
| return _isEuEndpoint(endpoint) ? STATS_SDK_CFG_URL_EU : STATS_SDK_CFG_URL_NON_EU; |
There was a problem hiding this comment.
We should cache the "endpoint" resolution to avoid running the _isEuEndpoint for every time _track is called.
| "language": STATS_LANGUAGE, | ||
| "version": internalSdkStatsStats.sdkVer || "unknown", | ||
| "endpoint": "breeze", | ||
| "host": _networkCounter.host |
There was a problem hiding this comment.
Some additional properties that are web specific that would be good to have reported as part of the SDK Stats is whether the SDK was loaded via the snippet and what version ( ai.context.internal.snippetVer``) and if it was loaded via CDN or npm (ai.context.internal.sdkSrc<- the CDN name it was loaded from) Set via the_findSdkSourceFile()` in AISku.ts (not currently setfor 1DS) -- we just have not have anywhere to report this before
There was a problem hiding this comment.
That's a great idea. Happy to brainstorm some more values that we could pass here. I can propose a spec PR for SDK Stats from there. How would you feel about that coming as a part of a follow up PR since any change should be additive?
| internalSdkStatsEvent.data = internalSdkStatsEvent.data || {}; | ||
| internalSdkStatsEvent.data[STATS_SDK_ENDPOINT_KEY] = url; | ||
|
|
||
| _core.track(internalSdkStatsEvent); |
There was a problem hiding this comment.
I don't think this is going to work??
As it looks like you are passing in the core from the AISku instance (which is the users configured instance), which will be batched and sent to the users endpoint (channel control).
It also means that when the end-users have their own telemetry initializers attached to the core to add / remove properties they may add / remove PII to the internal SDK events, also if they have not written their initializer correctly (they expect fields / properties that don't exist for a metric (the sdk stats event) it could cause an unhandled JavaScript exception. And I've seen some sites throw a complete page load failure error (causing their site to be declared "offline") on any unhandled exception.
While we need the root _core instance to monitor it for any changed config (loaded via the dynamic config or users changes their endpoint url), we should create instance and initialize our own instance of the SDK configured with the cfg.json endpoint.
which means that the init function should have an additional callback to support the creating of the SDK Instance (as not everyone uses core, properties and sender), so that for the main operation (SKU) we can control what is included (so AISku Lite there is no properties, for 1DS there is no Sender or AI Properties, so the SKU's in 1DS have to initialize themselves) etc.
Summary
Re-enables the SDK Stats manager (
createStatsMgr) and routes its events to the distro-owned SDK Stats ingestion endpoint used by opentelemetry-distro-dotnet, instead of piggy-backing on the customer's breeze endpoint.Endpoints (from
Azure/azure-sdk-for-netStatsbeatConstants.cshttps://stats.monitor.azure.com/v2/trackhttps://eu.stats.monitor.azure.com/v2/track00000000-0000-0000-0000-000000000000, same Breeze envelope, no auth. EU vs non-EU chosen from the customer endpoint region.Configurable Destination (new
stats.monitorendpoint vs. legacy breeze)The SDK Stats destination is now selectable at runtime via the CDN / dynamic config:
config.stats(IStatsBeatConfig), overridable via the CDN / cfgSync dynamic config.config.stats.mode(eStatsEndpointType):SdkStats(default) sends tostats.monitor.azure.com/eu.stats.monitor.azure.com;Breezesends to the legacy breeze endpoint (the customer's own breeze host, using the Microsoft-owned SDK Stats iKeyc4a29126…/7dc56bab…by region)._trackfrom the live config, so CDN changes take effect at runtime. A fullendCfgkey map (with explicitkeyand newurl) can also be supplied viaconfig.stats.endCfgto fully override routing.Note: in
Breezemode the events flow through the normal channel to the customer's breeze endpoint (legacy behaviour); inSdkStatsmode they bypass the customer buffer and are excluded from request counting.Changes
StatsBeat.ts— SDK Stats endpoint constants, EU/non-EU region detection (getStatsEndpoint), per-event destination + placeholder-iKey stamping, enabled-by-default feature gate (opt-out viafeatureOptIn: "sdkStats"),createSdkStatsMgrConfig()helper, and configurable mode + key map support.AppInsightsCore.ts/IAppInsightsCore.ts/index.ts— restoregetStatsBeat/setStatsMgr, fields, unload cleanup, stubs, interface methods, and exports.Sender.ts— restore request-counting hooks +statsBeatData; redirect SDK Stats items to the SDK Stats endpoint via a per-item URL override on_doSend/_getPayload, bypassing the customer buffer; SDK Stats sends are excluded from counting (no feedback loop); optimized property access for minification efficiency.AISku.ts— create + init + set the manager after core init (kept out of core/AISKULight for bundle size).IConfiguration.ts— addedstats?: IStatsBeatConfigfield for dynamic config.StatsType.ts— addedeStatsEndpointTypeenum (SdkStats/Breeze).IStatsBeat.ts— addedIStatsBeatConfig.modeandIStatsBeatKeyMap.urlfields.StatsBeat.Tests.tsin both core and channel-js), fixed import paths, updated terminology from "statsbeat" to "SDK Stats" in test names and assertions.Architecture note
createStatsMgrlives in core, which cannot depend on the channel's BreezeEnvelopeCreator. So SDK Stats events flow through_core.track()(reusing the channel's envelope creation) and the redirect to the SDK Stats endpoint happens in the Sender via a per-item endpoint override.Verification
tsc --noEmitfrom source using the repo's exact compiler settings — core, channel, and AISKU compile with 0 errors.Follow-ups (left for review)
AISKULightSize,AppInsightsCoreSize) if needed.npm run lint-fix+npm run fullCleanBuildbefore marking ready (requires environment outside VS Code due to Rush/ESLint plugin version conflicts).Co-authored-by: Copilot 223556219+Copilot@users.noreply.github.com