CAMS-709: Disable App Insights console auto-collection#2002
Draft
jamesobrooks wants to merge 8 commits intomainfrom
Draft
CAMS-709: Disable App Insights console auto-collection#2002jamesobrooks wants to merge 8 commits intomainfrom
jamesobrooks wants to merge 8 commits intomainfrom
Conversation
Contributor
Reviewer's GuideRefactors migrate-cases dataflow handlers to use a standardized ApplicationContext and ExtraOutputs abstraction while adding robust, explicitly initialized Application Insights telemetry configuration that disables console auto-collection and improves critical error logging when telemetry is unavailable. Sequence diagram for migrate-cases handler flow with ApplicationContext and ExtraOutputssequenceDiagram
participant AF as AzureFunctionHost
participant HC as handlePage
participant ACC as ApplicationContextCreator
participant AC as ApplicationContext
participant MC as MigrateCases
participant ELC as ExportAndLoadCase
participant Q as AzureStorageQueues
participant AI as ApplicationInsights
AF->>HC: invoke handlePage(range, invocationContext)
HC->>ACC: getApplicationContext(invocationContext, logger)
ACC-->>HC: ApplicationContext AC
HC->>AC: observability.startTrace(invocationId)
HC->>MC: getPageOfCaseEvents(AC, start, end)
MC-->>HC: CaseSyncEvent[] events
HC->>ELC: exportAndLoad(AC, events)
ELC-->>HC: processedEvents
HC->>AC: extraOutputs.set(DLQ, failedEvents)
HC->>AC: extraOutputs.set(...)
HC->>AC: observability.trackEvent(dataflow metrics)
AC->>AI: trackEvent(customEvent)
HC-->>AF: complete
Sequence diagram for explicit Application Insights initialization and configurationsequenceDiagram
participant DF as dataflows.ts
participant ACC as ApplicationContextCreator
participant CFG as configureAppInsights
participant GCF as getOrInitializeAppInsightsClient
participant AISDK as applicationinsightsSDK
participant AI as ApplicationInsights
participant LOG as LoggerImpl
DF->>CFG: configureAppInsights()
CFG->>AISDK: require(applicationinsights)
alt SDK not initialized yet
AISDK-->>CFG: defaultClient null
CFG-->>DF: return (no console config)
else SDK already initialized by host
CFG->>AISDK: Configuration.setAutoCollectConsole(false)
AISDK-->>CFG: console collection disabled
CFG-->>DF: return
end
ACC->>GCF: getOrInitializeAppInsightsClient(LOG)
GCF->>AISDK: require(applicationinsights)
alt defaultClient exists
AISDK-->>GCF: defaultClient
GCF-->>ACC: TelemetryClient
else defaultClient missing
GCF->>AISDK: setup(connectionString).start()
AISDK-->>GCF: defaultClient
GCF-->>ACC: TelemetryClient or null
end
ACC->>AI: use TelemetryClient for traces/events
Class diagram for updated ApplicationContext, ExtraOutputs, and AppInsightsObservabilityclassDiagram
class ExtraOutputs {
+set(output unknown, value unknown) void
}
class ApplicationContext {
+config ApplicationConfiguration
+featureFlags FeatureFlagSet
+observability ObservabilityGateway
+logger LoggerImpl
+invocationId string
+request CamsHttpRequest
+closables Closable[]
+releasables Releasable[]
+extraOutputs ExtraOutputs
}
class AppInsightsObservability {
-clientFactory AppInsightsClientFactory
-MODULE_NAME string
+constructor(logger LoggerImpl, clientFactory AppInsightsClientFactory)
+startTrace(invocationId string) ObservabilityTrace
+trackEvent(eventName string, properties Record)
+trackMetric(metricName string, value number, properties Record)
}
class TelemetryEvent {
+name string
+properties Record
}
class TelemetryMetric {
+name string
+value number
+properties Record
}
class TelemetryClient {
+trackEvent(event TelemetryEvent) void
+trackMetric(metric TelemetryMetric) void
}
class LoggerImpl
class ObservabilityGateway
class AppInsightsClientFactory
ApplicationContext --> ExtraOutputs : has
ApplicationContext --> ObservabilityGateway : uses
ApplicationContext --> LoggerImpl : uses
AppInsightsObservability ..|> ObservabilityGateway
AppInsightsObservability --> LoggerImpl : optional
AppInsightsObservability --> TelemetryClient : via clientFactory
AppInsightsObservability --> AppInsightsClientFactory : uses
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In
configureAppInsights, you setisConfigured = truewhen the module is falsy or throws, but not whendefaultClientis missing, which means the function will keep retrying and logging on every call in that scenario; consider settingisConfiguredthere as well if repeated retries aren’t useful. - Since
configureAppInsightsis invoked at module load in bothdataflows.tsandapplication-context-creator.ts, it might be worth clarifying in comments that the singletonisConfiguredflag lives inapp-insights.tsand ensures these multiple call sites won’t conflict or cause double configuration.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `configureAppInsights`, you set `isConfigured = true` when the module is falsy or throws, but not when `defaultClient` is missing, which means the function will keep retrying and logging on every call in that scenario; consider setting `isConfigured` there as well if repeated retries aren’t useful.
- Since `configureAppInsights` is invoked at module load in both `dataflows.ts` and `application-context-creator.ts`, it might be worth clarifying in comments that the singleton `isConfigured` flag lives in `app-insights.ts` and ensures these multiple call sites won’t conflict or cause double configuration.
## Individual Comments
### Comment 1
<location path="backend/function-apps/azure/app-insights.ts" line_range="45-46" />
<code_context>
+
+ // Disable console auto-collection to prevent duplicate logs
+ // The Functions host already captures logs via invocationContext.log()
+ if (appInsights.Configuration?.setAutoCollectConsole) {
+ appInsights.Configuration.setAutoCollectConsole(false);
+ console.log('[APP-INSIGHTS] Console auto-collection disabled to prevent duplicate logs');
+ }
</code_context>
<issue_to_address>
**question (bug_risk):** The `setAutoCollectConsole(false)` usage may not align with the Application Insights SDK API, which typically expects additional parameters.
In most versions of the Node.js SDK, `setAutoCollectConsole` is used on the config/setup chain with a signature like `setAutoCollectConsole(enabled: boolean, collectWarningsAndErrors?: boolean)`. Using `Configuration.setAutoCollectConsole(false)` directly may be a no-op depending on how this SDK version exposes configuration. Please:
- Verify that `appInsights.Configuration.setAutoCollectConsole` is a valid, supported entry point for your target SDK version, and
- Consider passing both arguments (e.g. `setAutoCollectConsole(false, false)`) or configuring via the documented chain (e.g. `appInsights.setup().setAutoCollectConsole(...)` or `defaultClient.config`).
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Azure Functions host logging and App Insights SDK console auto-collection were both capturing logs, causing duplicate entries in Application Insights. Added configureAppInsights() function to disable SDK console auto-collection at startup while preserving custom event/metric tracking. This ensures logs flow through only the Functions host path. Changes: - Created configureAppInsights() in app-insights.ts with singleton pattern to disable console auto-collection - Initialized in dataflows.ts at module load time - Initialized in application-context-creator.ts for API functions Jira ticket: CAMS-709 Co-authored-by: Claude <noreply@anthropic.com>
Explicitly initialize Application Insights SDK if defaultClient doesn't exist, resolving production issue where custom events and metrics were not being sent. Add getOrInitializeAppInsightsClient() that calls setup().start() when needed. Create ExtraOutputs interface as humble object abstraction for Azure Functions queue operations, replacing unknown type in ApplicationContext. Standardize all dataflow handlers to follow API function pattern: create logger early, create ApplicationContext once, use only appContext thereafter. Refactor helper functions to accept ApplicationContext instead of InvocationContext, eliminating multiple observability instances per invocation. Add CRITICAL error logging when telemetry fails to highlight business reporting impact for stakeholders. Jira ticket: CAMS-709 Co-authored-by: Prem Govinda <16512926+bityogi@users.noreply.github.com>
b3483b1 to
c1b55b5
Compare
Jira ticket: CAMS-709 Co-authored-by: Kelly D <41132296+diabagatekelly@users.noreply.github.com>
Extract repeated logger/context/trace initialization into withAppContextAndTrace helper function. Simplifies all four handlers (handleStart, handlePage, handleError, handleRetry) by removing ~60 lines of duplicated error handling and setup code. Addresses Sourcery code review feedback.
Reverted previous SDK-level fixes and instead disable console auto-collection via environment variable. This allows Azure Functions runtime to exclusively handle console log forwarding to App Insights, eliminating duplication. Changes: - Reverted app-insights.ts and observability.ts to original implementations - Added APPLICATIONINSIGHTS_ENABLE_CONSOLE_AUTO_COLLECTION=false to both API and dataflows Bicep templates Jira ticket: CAMS-709
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Purpose
Resolve production issue where custom events and metrics were not appearing in Application Insights, and eliminate inconsistent ApplicationContext usage patterns across dataflows.
Major Changes
getOrInitializeAppInsightsClient()callssetup().start()whendefaultClientis null, resolving production telemetry failuresunknowntype in ApplicationContextTesting/Validation
Notes
Historic customEvents from before this fix deployment will never be created, creating a permanent gap in pre-fix metrics. Post-fix data is accurate.
The ExtraOutputs interface and standardized ApplicationContext pattern should be applied to remaining dataflows not yet refactored.
Definition of Done:
Summary by Sourcery
Standardize observability and ApplicationContext usage in the migrate-cases dataflow while hardening Application Insights initialization and configuration to prevent duplicate logs and ensure telemetry is reliably captured.
New Features:
Bug Fixes:
Enhancements: