Skip to content

Commit 03d8c7f

Browse files
[backport core/1.51] fix(telemetry): route every error report through one reporter that reaches both sinks (#15464)
Backport of #15346 to `core/1.51` Automatically created by backport workflow. Co-authored-by: Christian Byrne <cbyrne@comfy.org>
1 parent 83fc697 commit 03d8c7f

16 files changed

Lines changed: 349 additions & 103 deletions

global.d.ts

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,5 @@
11
declare const __COMFYUI_FRONTEND_VERSION__: string
22
declare const __COMFYUI_FRONTEND_COMMIT__: string
3-
declare const __SENTRY_ENABLED__: boolean
43
declare const __SENTRY_DSN__: string
54
declare const __ALGOLIA_APP_ID__: string
65
declare const __ALGOLIA_API_KEY__: string

scripts/vite-define-shim.ts

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,6 @@
55

66
type GlobalWithDefines = typeof globalThis & {
77
__COMFYUI_FRONTEND_VERSION__: string
8-
__SENTRY_ENABLED__: boolean
98
__SENTRY_DSN__: string
109
__ALGOLIA_APP_ID__: string
1110
__ALGOLIA_API_KEY__: string
@@ -20,7 +19,6 @@ const globalWithDefines = globalThis as GlobalWithDefines
2019
// Set default values for Playwright test environment
2120
globalWithDefines.__COMFYUI_FRONTEND_VERSION__ =
2221
process.env.npm_package_version || '1.0.0'
23-
globalWithDefines.__SENTRY_ENABLED__ = false
2422
globalWithDefines.__SENTRY_DSN__ = ''
2523
globalWithDefines.__ALGOLIA_APP_ID__ = ''
2624
globalWithDefines.__ALGOLIA_API_KEY__ = ''

src/App.vue

Lines changed: 10 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -5,13 +5,13 @@
55
</template>
66

77
<script setup lang="ts">
8-
import { captureException } from '@sentry/vue'
98
import BlockUI from 'primevue/blockui'
109
import { computed, onMounted, watch } from 'vue'
1110
1211
import GlobalDialog from '@/components/dialog/GlobalDialog.vue'
1312
import config from '@/config'
1413
import { isDesktop } from '@/platform/distribution/types'
14+
import { reportError } from '@/platform/telemetry/reportError'
1515
import { app } from '@/scripts/app'
1616
import { useWorkspaceStore } from '@/stores/workspaceStore'
1717
import { electronAPI } from '@/utils/envUtil'
@@ -49,11 +49,9 @@ function handleResourceError(url: string, tagName: string) {
4949
console.error('[resource:loadError]', { url, tagName })
5050
5151
if (__DISTRIBUTION__ === 'cloud') {
52-
captureException(new Error(`Resource load failed: ${url}`), {
53-
tags: {
54-
error_type: 'resource_load_error',
55-
tag_name: tagName
56-
}
52+
reportError(new Error(`Resource load failed: ${url}`), {
53+
errorType: 'resource_load_error',
54+
tags: { tag_name: tagName }
5755
})
5856
}
5957
}
@@ -77,18 +75,16 @@ onMounted(() => {
7775
message: info.message
7876
})
7977
if (__DISTRIBUTION__ === 'cloud') {
80-
captureException(event.payload, {
78+
reportError(event.payload, {
79+
errorType: 'vite_preload_error',
8180
tags: {
82-
error_type: 'vite_preload_error',
8381
file_type: info.fileType,
8482
chunk_name: info.chunkName ?? undefined
8583
},
86-
contexts: {
87-
preload: {
88-
url: info.url,
89-
fileType: info.fileType,
90-
chunkName: info.chunkName
91-
}
84+
context: {
85+
url: info.url,
86+
fileType: info.fileType,
87+
chunkName: info.chunkName
9288
}
9389
})
9490
}

src/bootstrap.ts

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,9 @@
11
if (__DISTRIBUTION__ === 'cloud') {
22
const { initDatadogRum } = await import('@/platform/telemetry/initDatadogRum')
3-
void initDatadogRum().catch(() => {})
3+
const { flushErrorReports } = await import('@/platform/telemetry/reportError')
4+
void initDatadogRum()
5+
.then(flushErrorReports)
6+
.catch(() => {})
47
}
58

69
await import('./main')

src/main.ts

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@ import {
2020
remoteConfig
2121
} from '@/platform/remoteConfig/remoteConfig'
2222
import { syncHostUserIdWithFirebaseAuth } from '@/platform/telemetry/hostUserIdSync'
23+
import { flushErrorReports } from '@/platform/telemetry/reportError'
2324
import '@/lib/litegraph/public/css/litegraph.css'
2425
import router from '@/router'
2526
import { isDesktop, isNightly } from '@/platform/distribution/types'
@@ -67,10 +68,16 @@ const sentryDsn = isCloud
6768
? configValueOrDefault(remoteConfig.value, 'sentry_dsn', __SENTRY_DSN__)
6869
: __SENTRY_DSN__
6970

71+
// __SENTRY_ENABLED__ is baked from the *build machine's* SENTRY_DSN, but cloud
72+
// resolves its DSN at runtime from remote config. Trusting the build-time flag
73+
// alone leaves every capture in the app silently inert whenever a cloud build
74+
// runs without the env var, however valid the runtime DSN turns out to be.
75+
const sentryEnabled = !import.meta.env.DEV && !!sentryDsn
76+
7077
Sentry.init({
7178
app,
7279
dsn: sentryDsn,
73-
enabled: __SENTRY_ENABLED__,
80+
enabled: sentryEnabled,
7481
release: __COMFYUI_FRONTEND_VERSION__,
7582
normalizeDepth: 8,
7683
tracesSampleRate: isCloud ? 1.0 : 0,
@@ -92,6 +99,9 @@ Sentry.init({
9299
defaultIntegrations: false
93100
})
94101
})
102+
103+
flushErrorReports()
104+
95105
// Assertion reporter receives pre-formatted messages (with "[Assertion failed]: " prefix).
96106
// Strings here are intentionally not i18n'd: they're developer/nightly diagnostics,
97107
// not user-facing in stable releases.

src/platform/cloud/onboarding/auth.ts

Lines changed: 14 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
import * as Sentry from '@sentry/vue'
22
import { isEmpty } from 'es-toolkit/compat'
33

4+
import { reportError } from '@/platform/telemetry/reportError'
45
import { api } from '@/scripts/api'
56
import { toError } from '@/utils/errorUtil'
67

@@ -10,9 +11,6 @@ interface UserCloudStatus {
1011

1112
const ONBOARDING_SURVEY_KEY = 'onboarding_survey'
1213

13-
/**
14-
* Helper function to capture API errors with Sentry
15-
*/
1614
function captureApiError(
1715
error: Error,
1816
endpoint: string,
@@ -21,25 +19,15 @@ function captureApiError(
2119
operation?: string,
2220
extraContext?: Record<string, unknown>
2321
) {
24-
const tags: Record<string, string | number> = {
25-
api_endpoint: endpoint,
26-
error_type: errorType
27-
}
28-
29-
if (httpStatus !== undefined) {
30-
tags.http_status = httpStatus
31-
}
32-
33-
if (operation) {
34-
tags.operation = operation
35-
}
36-
37-
const sentryOptions: Sentry.ExclusiveEventHintOrCaptureContext = {
38-
tags,
39-
extra: extraContext ? { ...extraContext } : undefined
40-
}
41-
42-
Sentry.captureException(error, sentryOptions)
22+
reportError(error, {
23+
errorType,
24+
tags: {
25+
api_endpoint: endpoint,
26+
http_status: httpStatus,
27+
operation
28+
},
29+
context: extraContext
30+
})
4331
}
4432

4533
/**
@@ -119,12 +107,10 @@ export async function getSurveyCompletedStatus(): Promise<boolean> {
119107
return !isEmpty(data.value)
120108
} catch (error) {
121109
// Network/parse failure: same fail-safe policy as a non-ok response.
122-
Sentry.captureException(error, {
123-
tags: {
124-
api_endpoint: '/settings/{key}',
125-
error_type: 'network_error'
126-
},
127-
extra: {
110+
reportError(error, {
111+
errorType: 'network_error',
112+
tags: { api_endpoint: '/settings/{key}' },
113+
context: {
128114
route_template: '/settings/{key}',
129115
route_actual: `/settings/${ONBOARDING_SURVEY_KEY}`
130116
},
Lines changed: 167 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,167 @@
1+
import { beforeEach, describe, expect, it, vi } from 'vitest'
2+
3+
const captureException = vi.fn()
4+
const isEnabled = vi.fn()
5+
const addError = vi.fn()
6+
const getInitConfiguration = vi.fn()
7+
8+
vi.mock('@sentry/vue', () => ({
9+
captureException: (...args: unknown[]) => captureException(...args),
10+
isEnabled: () => isEnabled()
11+
}))
12+
13+
vi.mock('@datadog/browser-rum', () => ({
14+
datadogRum: {
15+
addError: (...args: unknown[]) => addError(...args),
16+
getInitConfiguration: () => getInitConfiguration()
17+
}
18+
}))
19+
20+
async function loadReportError() {
21+
vi.resetModules()
22+
return import('./reportError')
23+
}
24+
25+
const sentryLive = (live: boolean) => isEnabled.mockReturnValue(live)
26+
const datadogLive = (live: boolean) =>
27+
getInitConfiguration.mockReturnValue(live ? {} : undefined)
28+
29+
describe('reportError', () => {
30+
beforeEach(() => {
31+
vi.clearAllMocks()
32+
sentryLive(true)
33+
datadogLive(true)
34+
})
35+
36+
it('reaches both Sentry and Datadog from a single call', async () => {
37+
const { reportError } = await loadReportError()
38+
const error = new Error('boom')
39+
40+
reportError(error, {
41+
errorType: 'workspace_auth_gate_initialization_failure'
42+
})
43+
44+
expect(captureException).toHaveBeenCalledWith(
45+
error,
46+
expect.objectContaining({
47+
tags: expect.objectContaining({
48+
error_type: 'workspace_auth_gate_initialization_failure'
49+
})
50+
})
51+
)
52+
expect(addError).toHaveBeenCalledWith(
53+
error,
54+
expect.objectContaining({
55+
error_type: 'workspace_auth_gate_initialization_failure'
56+
})
57+
)
58+
})
59+
60+
it('still reports to Datadog when Sentry is inert', async () => {
61+
sentryLive(false)
62+
const { reportError } = await loadReportError()
63+
64+
reportError(new Error('boom'), { errorType: 'bootstrap_auth_wait_timeout' })
65+
66+
expect(captureException).not.toHaveBeenCalled()
67+
expect(addError).toHaveBeenCalledOnce()
68+
})
69+
70+
it('buffers reports raised before any sink is live, then flushes them', async () => {
71+
sentryLive(false)
72+
datadogLive(false)
73+
const { reportError, flushErrorReports } = await loadReportError()
74+
75+
reportError(new Error('early'), { errorType: 'resource_load_error' })
76+
expect(addError).not.toHaveBeenCalled()
77+
78+
datadogLive(true)
79+
flushErrorReports()
80+
81+
expect(addError).toHaveBeenCalledWith(
82+
expect.objectContaining({ message: 'early' }),
83+
expect.objectContaining({ error_type: 'resource_load_error' })
84+
)
85+
})
86+
87+
it('does not replay a buffered report twice', async () => {
88+
sentryLive(false)
89+
datadogLive(false)
90+
const { reportError, flushErrorReports } = await loadReportError()
91+
92+
reportError(new Error('early'), { errorType: 'resource_load_error' })
93+
94+
datadogLive(true)
95+
flushErrorReports()
96+
flushErrorReports()
97+
98+
expect(addError).toHaveBeenCalledOnce()
99+
})
100+
101+
it('bounds the buffer so a boot-time error storm cannot grow without limit', async () => {
102+
sentryLive(false)
103+
datadogLive(false)
104+
const { reportError, flushErrorReports } = await loadReportError()
105+
106+
for (let i = 0; i < 200; i++) {
107+
reportError(new Error(`e${i}`), { errorType: 'resource_load_error' })
108+
}
109+
110+
datadogLive(true)
111+
flushErrorReports()
112+
113+
expect(addError.mock.calls.length).toBeLessThanOrEqual(25)
114+
})
115+
116+
it('normalizes a non-Error cause', async () => {
117+
const { reportError } = await loadReportError()
118+
119+
reportError('just a string', { errorType: 'bootstrap_auth_wait_timeout' })
120+
121+
expect(addError).toHaveBeenCalledWith(
122+
expect.objectContaining({ message: 'just a string' }),
123+
expect.anything()
124+
)
125+
})
126+
127+
it('drops undefined tag values rather than forwarding them', async () => {
128+
const { reportError } = await loadReportError()
129+
130+
reportError(new Error('boom'), {
131+
errorType: 'http_error',
132+
tags: { api_endpoint: '/settings/{key}', http_status: undefined }
133+
})
134+
135+
const [, context] = addError.mock.calls[0]
136+
expect(context).not.toHaveProperty('http_status')
137+
expect(context).toMatchObject({ api_endpoint: '/settings/{key}' })
138+
})
139+
140+
it('does not throw out of flushErrorReports when a sink throws', async () => {
141+
sentryLive(false)
142+
datadogLive(false)
143+
const { reportError, flushErrorReports } = await loadReportError()
144+
145+
reportError(new Error('early'), { errorType: 'resource_load_error' })
146+
147+
datadogLive(true)
148+
addError.mockImplementation(() => {
149+
throw new Error('datadog exploded')
150+
})
151+
152+
expect(() => flushErrorReports()).not.toThrow()
153+
})
154+
155+
it('does not throw when a sink throws', async () => {
156+
captureException.mockImplementation(() => {
157+
throw new Error('sentry exploded')
158+
})
159+
const { reportError } = await loadReportError()
160+
161+
expect(() =>
162+
reportError(new Error('boom'), {
163+
errorType: 'bootstrap_auth_wait_timeout'
164+
})
165+
).not.toThrow()
166+
})
167+
})

0 commit comments

Comments
 (0)