From a90e354a812f7239c7ef03805b4d294320232362 Mon Sep 17 00:00:00 2001 From: Jackson Weber <47067795+JacksonWeber@users.noreply.github.com> Date: Fri, 8 Mar 2024 13:19:07 -0800 Subject: [PATCH] [Beta] Update Warning Tests and Add Console Logging by Default (#1292) * Update testing and default enable logger. * Update telemetryClient.ts * Update to use the warnings array. * Refactor how we handle logging errors in start/setup scenarios. * Fix warnings. * Update warning logger approach and setting for auto collect performance. * Ignore warning. --- src/shim/applicationinsights.ts | 20 +++-- src/shim/shim-config.ts | 38 +++++----- src/shim/telemetryClient.ts | 23 ++++-- .../shim/applicationInsights.tests.ts | 73 ++++++++----------- test/unitTests/shim/config.tests.ts | 68 ++++++++--------- test/unitTests/shim/testUtils.ts | 9 +++ 6 files changed, 121 insertions(+), 110 deletions(-) create mode 100644 test/unitTests/shim/testUtils.ts diff --git a/src/shim/applicationinsights.ts b/src/shim/applicationinsights.ts index 055424fde..ce4b14a1c 100644 --- a/src/shim/applicationinsights.ts +++ b/src/shim/applicationinsights.ts @@ -3,13 +3,13 @@ import * as http from "http"; import * as azureFunctionsTypes from "@azure/functions"; -import { SpanContext, diag } from "@opentelemetry/api"; +import { DiagConsoleLogger, SpanContext, diag } from "@opentelemetry/api"; import { Span } from "@opentelemetry/sdk-trace-base"; import { CorrelationContextManager } from "./correlationContextManager"; import { ICorrelationContext, HttpRequest, DistributedTracingModes } from "./types"; import { TelemetryClient } from "./telemetryClient"; import * as Contracts from "../declarations/contracts"; - +import { Util } from "../shared/util"; // We export these imports so that SDK users may use these classes directly. // They're exposed using "export import" so that types are passed along as expected @@ -32,7 +32,11 @@ export let defaultClient: TelemetryClient; * and start the SDK. */ export function setup(setupString?: string) { - defaultClient = new TelemetryClient(setupString); + if (!defaultClient) { + defaultClient = new TelemetryClient(setupString); + } else { + defaultClient.pushWarningToLog("Setup has already been called once. To set up a new client, please use TelemetryClient instead.") + } return Configuration; } @@ -44,10 +48,15 @@ export function setup(setupString?: string) { */ export function start() { try { - defaultClient.initialize(); + if (!defaultClient) { + diag.setLogger(new DiagConsoleLogger()); + diag.warn("Start cannot be called before setup. Please call setup() first."); + } else { + defaultClient.initialize(); + } return Configuration; } catch (error) { - diag.warn("The default client has not been initialized. Please make sure to call setup() before start()."); + diag.warn(`Failed to start default client: ${Util.getInstance().dumpObj(error)}`); } } @@ -139,6 +148,7 @@ export class Configuration { * @param collectExtendedMetrics if true, extended metrics counters will be collected every minute and sent to Application Insights * @returns {Configuration} this class */ + // eslint-disable-next-line @typescript-eslint/no-unused-vars public static setAutoCollectPerformance(value: boolean, collectExtendedMetrics: any) { if (defaultClient) { defaultClient.config.enableAutoCollectPerformance = value; diff --git a/src/shim/shim-config.ts b/src/shim/shim-config.ts index 11a144db8..f4de9dd20 100644 --- a/src/shim/shim-config.ts +++ b/src/shim/shim-config.ts @@ -61,18 +61,20 @@ class Config implements IConfig { public noPatchModules: string; public noDiagnosticChannel: boolean; + private _configWarnings: string[]; /** * Creates a new Config instance * @param setupString Connection String, instrumentationKey is no longer supported here */ - constructor(setupString?: string) { + constructor(setupString?: string, configWarnings?: string[]){ // Load config values from env variables and JSON if available this._mergeConfig(); this.connectionString = setupString; this.webInstrumentationConfig = this.webInstrumentationConfig || null; this.ignoreLegacyHeaders = true; this.webInstrumentationConnectionString = this.webInstrumentationConnectionString || ""; + this._configWarnings = configWarnings || []; } private _mergeConfig() { @@ -335,49 +337,51 @@ class Config implements IConfig { // NOT SUPPORTED CONFIGURATION OPTIONS if (this.disableAppInsights) { - diag.warn("disableAppInsights configuration no longer supported."); + this._configWarnings.push("disableAppInsights configuration no longer supported."); } - if (this.enableAutoCollectHeartbeat) { - diag.warn("Heartbeat metris are no longer supported."); + if (this.enableAutoCollectHeartbeat === true) { + this._configWarnings.push("Heartbeat metrics are no longer supported."); } if (this.enableAutoDependencyCorrelation === false) { - diag.warn("Auto dependency correlation cannot be turned off anymore."); + this._configWarnings.push("Auto dependency correlation cannot be turned off anymore."); } if (typeof (this.enableAutoCollectIncomingRequestAzureFunctions) === "boolean") { - diag.warn("Auto request generation in Azure Functions is no longer supported."); + this._configWarnings.push("Auto request generation in Azure Functions is no longer supported."); } if (this.enableUseAsyncHooks === false) { - diag.warn("The use of non async hooks is no longer supported."); + this._configWarnings.push("The use of non async hooks is no longer supported."); } if (this.distributedTracingMode === DistributedTracingModes.AI) { - diag.warn("AI only distributed tracing mode is no longer supported."); + this._configWarnings.push("AI only distributed tracing mode is no longer supported."); } if (this.enableResendInterval) { - diag.warn("The resendInterval configuration option is not supported by the shim."); + this._configWarnings.push("The resendInterval configuration option is not supported by the shim."); } if (this.enableMaxBytesOnDisk) { - diag.warn("The maxBytesOnDisk configuration option is not supported by the shim."); + this._configWarnings.push("The maxBytesOnDisk configuration option is not supported by the shim."); } if (this.ignoreLegacyHeaders === false) { - diag.warn("LegacyHeaders are not supported by the shim."); + this._configWarnings.push("LegacyHeaders are not supported by the shim."); } - if (this.maxBatchSize) { - diag.warn("The maxBatchSize configuration option is not supported by the shim."); + this._configWarnings.push("The maxBatchSize configuration option is not supported by the shim."); } if (this.enableLoggerErrorToTrace) { - diag.warn("The enableLoggerErrorToTrace configuration option is not supported by the shim."); + this._configWarnings.push("The enableLoggerErrorToTrace configuration option is not supported by the shim."); } if (this.httpAgent || this.httpsAgent) { - diag.warn("The httpAgent and httpsAgent configuration options are not supported by the shim."); + this._configWarnings.push("The httpAgent and httpsAgent configuration options are not supported by the shim."); } if ( this.webInstrumentationConfig || this.webInstrumentationSrc ) { - diag.warn("The webInstrumentation config and src options are not supported by the shim."); + this._configWarnings.push("The webInstrumentation config and src options are not supported by the shim."); } if (this.quickPulseHost) { - diag.warn("The quickPulseHost configuration option is not suppored by the shim."); + this._configWarnings.push("The quickPulseHost configuration option is not supported by the shim."); + } + if (this.correlationHeaderExcludedDomains) { + this._configWarnings.push("The correlationHeaderExcludedDomains configuration option is not supported by the shim."); } return options; } diff --git a/src/shim/telemetryClient.ts b/src/shim/telemetryClient.ts index ef0c17442..a33a465b4 100644 --- a/src/shim/telemetryClient.ts +++ b/src/shim/telemetryClient.ts @@ -22,21 +22,22 @@ import { AzureMonitorOpenTelemetryOptions } from "../types"; * and manually trigger immediate sending (flushing) */ export class TelemetryClient { + public context: Context; + public commonProperties: { [key: string]: string }; + public config: Config; private _attributeSpanProcessor: AttributeSpanProcessor; private _attributeLogProcessor: AttributeLogProcessor; private _logApi: LogApi; private _isInitialized: boolean; - public context: Context; - public commonProperties: { [key: string]: string }; - public config: Config; private _options: AzureMonitorOpenTelemetryOptions; + private _configWarnings: string[] = []; /** * Constructs a new instance of TelemetryClient * @param setupString the Connection String or Instrumentation Key to use (read from environment variable if not specified) */ constructor(input?: string) { - const config = new Config(input); + const config = new Config(input, this._configWarnings); this.config = config; this.commonProperties = {}; this.context = new Context(); @@ -57,7 +58,15 @@ export class TelemetryClient { this._attributeLogProcessor = new AttributeLogProcessor({ ...this.context.tags, ...this.commonProperties }); (logs.getLoggerProvider() as LoggerProvider).addLogRecordProcessor(this._attributeLogProcessor); - } catch (error) { + + // Warn if any config warnings were generated during parsing + for (let i = 0; i < this._configWarnings.length; i++) { + diag.warn(this._configWarnings[i]); + this._attributeLogProcessor = new AttributeLogProcessor({ ...this.context.tags, ...this.commonProperties }); + (logs.getLoggerProvider() as LoggerProvider).addLogRecordProcessor(this._attributeLogProcessor); + } + } + catch (error) { diag.error(`Failed to initialize TelemetryClient ${error}`); } } @@ -315,4 +324,8 @@ export class TelemetryClient { public async shutdown(): Promise { return shutdownAzureMonitor(); } + + public pushWarningToLog(warning: string) { + this._configWarnings.push(warning); + } } diff --git a/test/unitTests/shim/applicationInsights.tests.ts b/test/unitTests/shim/applicationInsights.tests.ts index 12cc38966..03052496b 100644 --- a/test/unitTests/shim/applicationInsights.tests.ts +++ b/test/unitTests/shim/applicationInsights.tests.ts @@ -4,8 +4,9 @@ import * as assert from "assert"; import * as sinon from "sinon"; import * as appInsights from "../../../src/index"; -import { diag, DiagLogLevel } from "@opentelemetry/api"; +import { diag } from "@opentelemetry/api"; import { InstrumentationOptions } from "../../../src/types"; +import { checkWarnings } from "./testUtils"; describe("ApplicationInsights", () => { let sandbox: sinon.SinonSandbox; @@ -21,21 +22,25 @@ describe("ApplicationInsights", () => { describe("#setup()", () => { it("should not warn if setup is called once", (done) => { - const warnStub = sandbox.stub(console, "warn"); appInsights.setup(connString); - assert.ok(warnStub.notCalled, "warning was raised"); + const warnings = appInsights.defaultClient["_configWarnings"]; + assert.ok(!checkWarnings("Setup has already been called once", warnings), "setup warning was incorrectly raised"); done(); }); it("should warn if setup is called twice", (done) => { - const warnStub = sandbox.stub(console, "warn"); + const warnStub = sandbox.stub(diag, "warn"); appInsights.setup(connString); appInsights.setup(connString); - assert.ok(warnStub.calledOn, "warning was not raised"); + warnStub.args.forEach((arg) => { + if (arg.includes("Setup has already been called once")) { + assert.ok(true, "setup warning was not raised"); + } + }); done(); }); it("should not overwrite default client if called more than once", (done) => { appInsights.setup(connString); - var client = appInsights.defaultClient; + const client = appInsights.defaultClient; appInsights.setup(connString); appInsights.setup(connString); appInsights.setup(connString); @@ -46,16 +51,20 @@ describe("ApplicationInsights", () => { describe("#start()", () => { it("should warn if start is called before setup", (done) => { - const warnStub = sandbox.stub(console, "warn"); + const warnStub = sandbox.stub(diag, "warn"); appInsights.start(); - assert.ok(warnStub.calledOn, "warning was not raised"); + warnStub.args.forEach((arg) => { + if (arg.includes("Start cannot be called before setup. Please call setup() first.")) { + assert.ok(true, "setup warning was not raised"); + } + }); done(); }); it("should not warn if start is called after setup", () => { - var warnStub = sandbox.stub(console, "warn"); appInsights.setup(connString).start(); - assert.ok(warnStub.notCalled, "warning was raised"); + const warnings = appInsights.defaultClient["_configWarnings"]; + assert.ok(!checkWarnings("Start cannot be called before setup. Please call setup() first.", warnings), "warning was thrown when start was called correctly"); }); }); @@ -93,44 +102,28 @@ describe("ApplicationInsights", () => { }); describe("#Configuration", () => { - it("should throw warning if attempting to set AI distributed tracing mode", () => { - const warnStub = sandbox.stub(console, "warn"); + it("should throw warning if attempting to set AI distributed tracing mode to AI", () => { appInsights.setup(connString); - appInsights.Configuration.setDistributedTracingMode(appInsights.DistributedTracingModes.AI_AND_W3C); + const warnings = appInsights.defaultClient["_configWarnings"]; + appInsights.Configuration.setDistributedTracingMode(appInsights.DistributedTracingModes.AI); appInsights.start(); - assert.ok(warnStub.calledOn, "warning was not raised"); - }); - - it("should warn if attempting to set auto collection of pre-aggregated metrics", () => { - const warnStub = sandbox.stub(console, "warn"); - appInsights.setup(connString); - appInsights.Configuration.setAutoCollectPreAggregatedMetrics(true); - appInsights.start(); - assert.ok(warnStub.calledOn, "warning was not raised"); + assert.ok(checkWarnings("AI only distributed tracing mode is no longer supported.", warnings), "warning was not raised"); }); it("should warn if attempting to set auto collection of heartbeat", () => { - const warnStub = sandbox.stub(console, "warn"); appInsights.setup(connString); + const warnings = appInsights.defaultClient["_configWarnings"]; appInsights.Configuration.setAutoCollectHeartbeat(true); appInsights.start(); - assert.ok(warnStub.calledOn, "warning was not raised"); - }); - - it("should warn if attempting to enable web instrumentation", () => { - const warnStub = sandbox.stub(console, "warn"); - appInsights.setup(connString); - appInsights.Configuration.enableWebInstrumentation(true); - appInsights.start(); - assert.ok(warnStub.calledOn, "warning was not raised"); + assert.ok(checkWarnings("Heartbeat metrics are no longer supported.", warnings), "warning was not raised"); }); it("should warn if attempting to set maxBytesOnDisk", () => { - const warnStub = sandbox.stub(console, "warn"); appInsights.setup(connString); + const warnings = appInsights.defaultClient["_configWarnings"]; appInsights.Configuration.setUseDiskRetryCaching(true, 1000, 10); appInsights.start(); - assert.ok(warnStub.calledOn, "warning was not raised"); + assert.ok(checkWarnings("The maxBytesOnDisk configuration option is not supported by the shim.", warnings), "warning was not raised"); }); it("should set internal loggers", () => { @@ -141,19 +134,11 @@ describe("ApplicationInsights", () => { }); it("should warn if attempting to auto collect incoming azure functions requests", () => { - const warnStub = sandbox.stub(console, "warn"); appInsights.setup(connString); + const warnings = appInsights.defaultClient["_configWarnings"]; appInsights.Configuration.setAutoCollectIncomingRequestAzureFunctions(true); appInsights.start(); - assert.ok(warnStub.calledOn, "warning was not raised"); - }); - - it("should warn if attempting to send live metrics", () => { - const warnStub = sandbox.stub(console, "warn"); - appInsights.setup(connString); - appInsights.Configuration.setSendLiveMetrics(true); - appInsights.start(); - assert.ok(warnStub.calledOn, "warning was not raised"); + assert.ok(checkWarnings("Auto request generation in Azure Functions is no longer supported.", warnings), "warning was not raised"); }); }); }); diff --git a/test/unitTests/shim/config.tests.ts b/test/unitTests/shim/config.tests.ts index 31cec769b..f26a487f5 100644 --- a/test/unitTests/shim/config.tests.ts +++ b/test/unitTests/shim/config.tests.ts @@ -9,7 +9,7 @@ import { TelemetryClient } from "../../../src/shim/telemetryClient"; import http = require("http"); import https = require("https"); import { DistributedTracingModes } from '../../../applicationinsights'; - +import { checkWarnings } from './testUtils'; class TestTokenCredential implements azureCoreAuth.TokenCredential { private _expiresOn: Date; @@ -224,127 +224,117 @@ describe("shim/configuration/config", () => { describe("#Shim unsupported messages", () => { it("should warn if disableAppInsights is set", () => { - const warnStub = sandbox.stub(console, "warn"); const config = new Config(connectionString); + const warnings = config["_configWarnings"]; config.disableAppInsights = true; config.parseConfig(); - assert.ok(warnStub.calledOn, "warning was not raised"); + assert.ok(checkWarnings("disableAppInsights configuration no longer supported.", warnings), "warning was not raised"); }); it("should warn if collect heartbeat is set", () => { - const warnStub = sandbox.stub(console, "warn"); const config = new Config(connectionString); + const warnings = config["_configWarnings"]; config.enableAutoCollectHeartbeat = true; config.parseConfig(); - assert.ok(warnStub.calledOn, "warning was not raised"); + assert.ok(checkWarnings("Heartbeat metrics are no longer supported.", warnings), "warning was not raised"); }); it("should warn if auto dependency correlation is set to false", () => { - const warnStub = sandbox.stub(console, "warn"); const config = new Config(connectionString); + const warnings = config["_configWarnings"]; config.enableAutoDependencyCorrelation = false; config.parseConfig(); - assert.ok(warnStub.calledOn, "warning was not raised"); + assert.ok(checkWarnings("Auto dependency correlation cannot be turned off anymore.", warnings), "warning was not raised"); }); it("should warn if auto request generation is azure functions is set", () => { - const warnStub = sandbox.stub(console, "warn"); const config = new Config(connectionString); + const warnings = config["_configWarnings"]; config.enableAutoCollectIncomingRequestAzureFunctions = true; config.parseConfig(); - assert.ok(warnStub.calledOn, "warning was not raised"); - }); - - it("should warn if live metrics is set", () => { - const warnStub = sandbox.stub(console, "warn"); - const config = new Config(connectionString); - config.enableSendLiveMetrics = true; - config.parseConfig(); - assert.ok(warnStub.calledOn, "warning was not raised"); + assert.ok(checkWarnings("Auto request generation in Azure Functions is no longer supported.", warnings), "warning was not raised"); }); it("should warn if using async hooks is set to false", () => { - const warnStub = sandbox.stub(console, "warn"); const config = new Config(connectionString); + const warnings = config["_configWarnings"]; config.enableUseAsyncHooks = false; config.parseConfig(); - assert.ok(warnStub.calledOn, "warning was not raised"); + assert.ok(checkWarnings("The use of non async hooks is no longer supported.", warnings), "warning was not raised"); }); it("should warn if distributed tracing mode is set to AI", () => { - const warnStub = sandbox.stub(console, "warn"); const config = new Config(connectionString); + const warnings = config["_configWarnings"]; config.distributedTracingMode = DistributedTracingModes.AI; config.parseConfig(); - assert.ok(warnStub.calledOn, "warning was not raised"); + assert.ok(checkWarnings("AI only distributed tracing mode is no longer supported.", warnings), "warning was not raised"); }); it("should warn if resend interval is set", () => { - const warnStub = sandbox.stub(console, "warn"); const config = new Config(connectionString); + const warnings = config["_configWarnings"]; config.enableResendInterval = 1; config.parseConfig(); - assert.ok(warnStub.calledOn, "warning was not raised"); + assert.ok(checkWarnings("The resendInterval configuration option is not supported by the shim.", warnings), "warning was not raised"); }); it("should warn if max bytes on disk is set", () => { - const warnStub = sandbox.stub(console, "warn"); const config = new Config(connectionString); + const warnings = config["_configWarnings"]; config.enableMaxBytesOnDisk = 1; config.parseConfig(); - assert.ok(warnStub.calledOn, "warning was not raised"); + assert.ok(checkWarnings("The maxBytesOnDisk configuration option is not supported by the shim.", warnings), "warning was not raised"); }); it("should warn if ignore legacy headers is false", () => { - const warnStub = sandbox.stub(console, "warn"); const config = new Config(connectionString); + const warnings = config["_configWarnings"]; config.ignoreLegacyHeaders = false; config.parseConfig(); - assert.ok(warnStub.calledOn, "warning was not raised"); + assert.ok(checkWarnings("LegacyHeaders are not supported by the shim.", warnings), "warning was not raised"); }); it("should warn if max batch size is set", () => { - const warnStub = sandbox.stub(console, "warn"); const config = new Config(connectionString); + const warnings = config["_configWarnings"]; config.maxBatchSize = 1; config.parseConfig(); - assert.ok(warnStub.calledOn, "warning was not raised"); + assert.ok(checkWarnings("The maxBatchSize configuration option is not supported by the shim.", warnings), "warning was not raised"); }); it("should warn if logger errors are set to traces", () => { - const warnStub = sandbox.stub(console, "warn"); const config = new Config(connectionString); + const warnings = config["_configWarnings"]; config.enableLoggerErrorToTrace = true; config.parseConfig(); - assert.ok(warnStub.calledOn, "warning was not raised"); + assert.ok(checkWarnings("The enableLoggerErrorToTrace configuration option is not supported by the shim.", warnings), "warning was not raised"); }); it("should warn if httpAgent or httpsAgent are set", () => { - const warnStub = sandbox.stub(console, "warn"); const config = new Config(connectionString); + const warnings = config["_configWarnings"]; config.httpAgent = new http.Agent(); config.httpsAgent = new https.Agent(); config.parseConfig(); - assert.ok(warnStub.calledOn, "warning was not raised"); + assert.ok(checkWarnings("The httpAgent and httpsAgent configuration options are not supported by the shim.", warnings), "warning was not raised"); }); it("should warn if web instrumentations are set", () => { - const warnStub = sandbox.stub(console, "warn"); const config = new Config(connectionString); - config.enableWebInstrumentation = true; + const warnings = config["_configWarnings"]; config.webInstrumentationConfig = [{name: "test", value: true}]; config.webInstrumentationSrc = "test"; - config.webInstrumentationConnectionString = "test"; config.parseConfig(); - assert.ok(warnStub.calledOn, "warning was not raised"); + assert.ok(checkWarnings("The webInstrumentation config and src options are not supported by the shim.", warnings), "warning was not raised"); }); it("should warn if correlationHeaderExcludedDomains is set", () => { - const warnStub = sandbox.stub(console, "warn"); const config = new Config(connectionString); + const warnings = config["_configWarnings"]; config.correlationHeaderExcludedDomains = ["test.com"]; config.parseConfig(); - assert.ok(warnStub.calledOn, "warning was not raised"); + assert.ok(checkWarnings("The correlationHeaderExcludedDomains configuration option is not supported by the shim.", warnings), "warning was not raised"); }); }); }); diff --git a/test/unitTests/shim/testUtils.ts b/test/unitTests/shim/testUtils.ts new file mode 100644 index 000000000..f2f6009b3 --- /dev/null +++ b/test/unitTests/shim/testUtils.ts @@ -0,0 +1,9 @@ +export function checkWarnings(warning: string, warnings: string[]) { + let expectedWarning: any; + for (let i = 0; i < warnings.length; i++) { + if (warnings[i].toString().includes(warning)) { + expectedWarning = warnings[i]; + } + } + return expectedWarning; +}