Skip to content

Commit 9ed6655

Browse files
JacksonWeberJacksonWeber
committed
Address github comments.
1 parent 9ae5c63 commit 9ed6655

4 files changed

Lines changed: 78 additions & 36 deletions

File tree

‎AISKU/src/AISku.ts‎

Lines changed: 12 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -404,10 +404,7 @@ export class AppInsightsSku implements IApplicationInsights<IConfiguration & ICo
404404
},
405405
lang: "JavaScript",
406406
ver: SDK_STATS_VERSION,
407-
int: SDK_STATS_FLUSH_INTERVAL,
408-
fnFlush: function () {
409-
_self.onunloadFlush(false);
410-
}
407+
int: SDK_STATS_FLUSH_INTERVAL
411408
});
412409
_core.addNotificationListener(_sdkStatsListener);
413410
}
@@ -614,19 +611,24 @@ export class AppInsightsSku implements IApplicationInsights<IConfiguration & ICo
614611
}
615612
}
616613

617-
_self.onunloadFlush(isAsync);
618-
619614
_removePageEventHandlers();
620615

621-
// Unload SDK Stats listener BEFORE core.unload() tears down the pipeline.
616+
// Unload SDK Stats listener BEFORE flushing the channel.
622617
// unload() calls core.track() to enqueue the accumulated metrics,
623-
// then calls fnFlush to flush the channel so they actually get sent.
618+
// then the single onunloadFlush below sends everything (regular telemetry + SDK stats).
624619
if (_sdkStatsListener && _core) {
625-
_sdkStatsListener.unload();
626-
_core.removeNotificationListener(_sdkStatsListener);
620+
try {
621+
_sdkStatsListener.unload();
622+
_core.removeNotificationListener(_sdkStatsListener);
623+
} catch (e) {
624+
// Swallow any errors to ensure core.unload() is still called
625+
}
627626
_sdkStatsListener = null;
628627
}
629628

629+
// Single flush sends both regular telemetry AND SDK stats metrics
630+
_self.onunloadFlush(isAsync);
631+
630632
_core.unload && _core.unload(isAsync, _unloadCallback, cbTimeout);
631633

632634
return result;

‎channels/applicationinsights-channel-js/src/Sender.ts‎

Lines changed: 9 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -627,12 +627,12 @@ export class Sender extends BaseTelemetryPlugin implements IChannelControls {
627627
/**
628628
* error handler
629629
*/
630-
_self._onError = (payload: IInternalStorageItem[] | string[], message: string, event?: ErrorEvent) => {
630+
_self._onError = (payload: IInternalStorageItem[] | string[], message: string, event?: ErrorEvent, statusCode?: number) => {
631631
// since version 3.1.3, string[] is no-op
632632
if (_isStringArr(payload)) {
633633
return;
634634
}
635-
return _onError(payload as IInternalStorageItem[], message, event);
635+
return _onError(payload as IInternalStorageItem[], message, event, statusCode);
636636
};
637637

638638
/**
@@ -800,7 +800,7 @@ export class Sender extends BaseTelemetryPlugin implements IChannelControls {
800800
/**
801801
* error handler
802802
*/
803-
function _onError(payload: IInternalStorageItem[], message: string, event?: ErrorEvent) {
803+
function _onError(payload: IInternalStorageItem[], message: string, event?: ErrorEvent, statusCode?: number) {
804804
_throwInternal(_self.diagLog(),
805805
eLoggingSeverity.WARNING,
806806
_eInternalMessageId.OnError,
@@ -814,7 +814,7 @@ export class Sender extends BaseTelemetryPlugin implements IChannelControls {
814814
if (mgr) {
815815
let items = _extractTelemetryItems(payload);
816816
if (items) {
817-
mgr.eventsDiscarded(items, 1 /* NonRetryableStatus */);
817+
mgr.eventsDiscarded(items, 1 /* NonRetryableStatus */, statusCode);
818818
}
819819
}
820820
}
@@ -1133,7 +1133,7 @@ export class Sender extends BaseTelemetryPlugin implements IChannelControls {
11331133
// Updates the end point url before retry
11341134
if(status === 301 || status === 307 || status === 308) {
11351135
if(!_checkAndUpdateEndPointUrl(responseUrl)) {
1136-
_self._onError(payload, errorMessage);
1136+
_self._onError(payload, errorMessage, undefined, status);
11371137
return;
11381138
}
11391139
}
@@ -1164,7 +1164,7 @@ export class Sender extends BaseTelemetryPlugin implements IChannelControls {
11641164
_eInternalMessageId.TransmissionFailed, ". " +
11651165
"Response code " + status + ". Will retry to send " + payload.length + " items.");
11661166
} else {
1167-
_self._onError(payload, errorMessage);
1167+
_self._onError(payload, errorMessage, undefined, status);
11681168
}
11691169
} else {
11701170
// check if the xhr's responseURL or fetch's response.url is same as endpoint url
@@ -1179,7 +1179,7 @@ export class Sender extends BaseTelemetryPlugin implements IChannelControls {
11791179
if (response && !_isRetryDisabled) {
11801180
_self._onPartialSuccess(payload, response);
11811181
} else {
1182-
_self._onError(payload, errorMessage);
1182+
_self._onError(payload, errorMessage, undefined, status);
11831183
}
11841184
} else {
11851185
_consecutiveErrors = 0;
@@ -1423,7 +1423,8 @@ export class Sender extends BaseTelemetryPlugin implements IChannelControls {
14231423
let items: ITelemetryItem[] = [];
14241424
arrForEach(payload, (p) => {
14251425
if (p) {
1426-
items.push({ name: "", baseType: p.bT || "EventData" } as ITelemetryItem);
1426+
let baseType = p.bT || "EventData";
1427+
items.push({ name: baseType, baseType: baseType } as ITelemetryItem);
14271428
}
14281429
});
14291430
return items.length ? items : null;

‎shared/AppInsightsCore/Tests/Unit/src/ai/SdkStatsNotificationCbk.Tests.ts‎

Lines changed: 57 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -5,13 +5,11 @@ import { NotificationManager } from "../../../../src/core/NotificationManager";
55

66
export class SdkStatsNotificationCbkTests extends AITestClass {
77
private _trackedItems: ITelemetryItem[];
8-
private _flushCalled: boolean;
98
private _listener: ISdkStatsNotifCbk;
109

1110
public testInitialize() {
1211
super.testInitialize();
1312
this._trackedItems = [];
14-
this._flushCalled = false;
1513
this._listener = null;
1614
}
1715

@@ -22,7 +20,6 @@ export class SdkStatsNotificationCbkTests extends AITestClass {
2220
this._listener = null;
2321
}
2422
this._trackedItems = [];
25-
this._flushCalled = false;
2623
}
2724

2825
public registerTests() {
@@ -31,6 +28,7 @@ export class SdkStatsNotificationCbkTests extends AITestClass {
3128
this._testEventsDiscarded();
3229
this._testEventsRetry();
3330
this._testFlush();
31+
this._testTimerBasedFlush();
3432
this._testUnload();
3533
this._testBaseTypeMapping();
3634
this._testSdkStatsMetricFiltering();
@@ -45,10 +43,7 @@ export class SdkStatsNotificationCbkTests extends AITestClass {
4543
},
4644
lang: "JavaScript",
4745
ver: "3.3.6",
48-
int: 100, // short interval for testing
49-
fnFlush: function () {
50-
_self._flushCalled = true;
51-
}
46+
int: 100 // short interval for testing
5247
};
5348

5449
if (overrides) {
@@ -309,9 +304,62 @@ export class SdkStatsNotificationCbkTests extends AITestClass {
309304
});
310305
}
311306

307+
private _testTimerBasedFlush() {
308+
this.testCase({
309+
name: "SdkStatsNotifCbk: metrics are automatically flushed after the configured timer interval",
310+
useFakeTimers: true,
311+
test: () => {
312+
let listener = this._createListener(); // interval = 100ms
313+
314+
// Queue some events — this starts the internal timer
315+
listener.eventsSent([
316+
this._makeItem("EventData"),
317+
this._makeItem("ExceptionData")
318+
]);
319+
320+
// No flush called yet — nothing should have been emitted
321+
Assert.equal(0, this._trackedItems.length, "No metrics should be emitted before timer fires");
322+
323+
// Advance the clock past the configured interval (100ms)
324+
this.clock.tick(101);
325+
326+
// The timer should have fired and flushed the accumulated counts
327+
Assert.equal(2, this._trackedItems.length, "Metrics should be emitted after timer fires");
328+
329+
let names = this._trackedItems.map(function (item) { return item.name; });
330+
Assert.ok(names.indexOf("Item_Success_Count") >= 0, "Should contain Item_Success_Count");
331+
}
332+
});
333+
334+
this.testCase({
335+
name: "SdkStatsNotifCbk: timer resets after flush and accumulates next interval independently",
336+
useFakeTimers: true,
337+
test: () => {
338+
let listener = this._createListener(); // interval = 100ms
339+
340+
// First interval
341+
listener.eventsSent([this._makeItem("EventData")]);
342+
this.clock.tick(101);
343+
344+
Assert.equal(1, this._trackedItems.length, "First interval should emit 1 metric");
345+
Assert.equal(1, this._trackedItems[0].baseData.average, "First interval count should be 1");
346+
347+
// Reset tracking for second interval
348+
this._trackedItems = [];
349+
350+
// Second interval — new events
351+
listener.eventsSent([this._makeItem("EventData"), this._makeItem("EventData")]);
352+
this.clock.tick(101);
353+
354+
Assert.equal(1, this._trackedItems.length, "Second interval should emit 1 metric");
355+
Assert.equal(2, this._trackedItems[0].baseData.average, "Second interval count should be 2");
356+
}
357+
});
358+
}
359+
312360
private _testUnload() {
313361
this.testCase({
314-
name: "SdkStatsNotifCbk: unload flushes remaining counts and calls fnFlush",
362+
name: "SdkStatsNotifCbk: unload flushes remaining counts",
315363
test: () => {
316364
let listener = this._createListener();
317365

@@ -321,20 +369,18 @@ export class SdkStatsNotificationCbkTests extends AITestClass {
321369
this._listener = null;
322370

323371
Assert.equal(1, this._trackedItems.length, "Should flush remaining counts on unload");
324-
Assert.ok(this._flushCalled, "fnFlush should be called on unload");
325372
}
326373
});
327374

328375
this.testCase({
329-
name: "SdkStatsNotifCbk: unload with no pending data still calls fnFlush",
376+
name: "SdkStatsNotifCbk: unload with no pending data emits nothing",
330377
test: () => {
331378
let listener = this._createListener();
332379

333380
listener.unload();
334381
this._listener = null;
335382

336383
Assert.equal(0, this._trackedItems.length, "Should not emit any metrics when no data");
337-
Assert.ok(this._flushCalled, "fnFlush should still be called");
338384
}
339385
});
340386
}

‎shared/AppInsightsCore/src/core/SdkStatsNotificationCbk.ts‎

Lines changed: 0 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -56,11 +56,6 @@ export interface ISdkStatsConfig {
5656
* Flush interval override in ms (default 900000 = 15 min).
5757
*/
5858
int?: number;
59-
/**
60-
* Optional callback to flush the channel after enqueuing SDK stats metrics.
61-
* Called by unload() after core.track() so the metrics get transmitted before teardown.
62-
*/
63-
fnFlush?: () => void;
6459
}
6560

6661
/**
@@ -269,8 +264,6 @@ export function createSdkStatsNotifCbk(cfg: ISdkStatsConfig): ISdkStatsNotifCbk
269264
unload: function () {
270265
// Flush remaining counts before unload
271266
_flush();
272-
// Flush the channel so the metrics just enqueued actually get sent
273-
cfg.fnFlush && cfg.fnFlush();
274267
if (_timer) {
275268
_timer.cancel();
276269
_timer = null;

0 commit comments

Comments
 (0)