Skip to content

Commit 5c05bd8

Browse files
mydeamydea
authored
fix(utils): Keep logger on carrier (#13570)
Today, if you have a pluggable integration, it will incorrectly use it's own logger, which will generally be disabled. This is because we inline all the logger code into the bundle, but the logger is never enabled. Now, the logger is kept as a singleton on the carrier, ensuring that we always use the same logger. I added browser integration tests to cover this - especially one in feedback which failed before. NOTE: This is still not really ideal, because we still include the whole `makeLogger` code in each pluggable integration that uses the logger 🤔 but we can look into this later - now, at least logging should work for pluggable integrations.
1 parent d929462 commit 5c05bd8

File tree

13 files changed

+225
-86
lines changed

13 files changed

+225
-86
lines changed

‎dev-packages/browser-integration-tests/suites/feedback/attachTo/init.js

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,17 @@
1+
import * as Sentry from '@sentry/browser';
2+
// Import this separately so that generatePlugin can handle it for CDN scenarios
3+
import { feedbackIntegration } from '@sentry/browser';
4+
5+
const feedback = feedbackIntegration({
6+
autoInject: false,
7+
});
8+
9+
window.Sentry = Sentry;
10+
window.feedback = feedback;
11+
12+
Sentry.init({
13+
dsn: 'https://[email protected]/1337',
14+
integrations: [feedback],
15+
});
16+
17+
feedback.attachTo('#custom-feedback-buttom');

‎dev-packages/browser-integration-tests/suites/feedback/attachTo/template.html

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,9 @@
1+
<!DOCTYPE html>
2+
<html>
3+
<head>
4+
<meta charset="utf-8" />
5+
</head>
6+
<body>
7+
<button type="button" id="custom-feedback-buttom">Show feedback!</button>
8+
</body>
9+
</html>

‎dev-packages/browser-integration-tests/suites/feedback/attachTo/test.ts

Lines changed: 82 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,82 @@
1+
import { expect } from '@playwright/test';
2+
3+
import { TEST_HOST, sentryTest } from '../../../utils/fixtures';
4+
import { envelopeRequestParser, getEnvelopeType, shouldSkipFeedbackTest } from '../../../utils/helpers';
5+
6+
sentryTest('should capture feedback with custom button', async ({ getLocalTestUrl, page }) => {
7+
if (shouldSkipFeedbackTest()) {
8+
sentryTest.skip();
9+
}
10+
11+
const feedbackRequestPromise = page.waitForResponse(res => {
12+
const req = res.request();
13+
14+
const postData = req.postData();
15+
if (!postData) {
16+
return false;
17+
}
18+
19+
try {
20+
return getEnvelopeType(req) === 'feedback';
21+
} catch (err) {
22+
return false;
23+
}
24+
});
25+
26+
await page.route('https://dsn.ingest.sentry.io/**/*', route => {
27+
return route.fulfill({
28+
status: 200,
29+
contentType: 'application/json',
30+
body: JSON.stringify({ id: 'test-id' }),
31+
});
32+
});
33+
34+
const url = await getLocalTestUrl({ testDir: __dirname });
35+
36+
await page.goto(url);
37+
await page.locator('#custom-feedback-buttom').click();
38+
await page.waitForSelector(':visible:text-is("Report a Bug")');
39+
40+
expect(await page.locator(':visible:text-is("Report a Bug")').count()).toEqual(1);
41+
await page.locator('[name="name"]').fill('Jane Doe');
42+
await page.locator('[name="email"]').fill('[email protected]');
43+
await page.locator('[name="message"]').fill('my example feedback');
44+
await page.locator('[data-sentry-feedback] .btn--primary').click();
45+
46+
const feedbackEvent = envelopeRequestParser((await feedbackRequestPromise).request());
47+
expect(feedbackEvent).toEqual({
48+
type: 'feedback',
49+
breadcrumbs: expect.any(Array),
50+
contexts: {
51+
feedback: {
52+
contact_email: '[email protected]',
53+
message: 'my example feedback',
54+
name: 'Jane Doe',
55+
source: 'widget',
56+
url: `${TEST_HOST}/index.html`,
57+
},
58+
trace: {
59+
trace_id: expect.stringMatching(/\w{32}/),
60+
span_id: expect.stringMatching(/\w{16}/),
61+
},
62+
},
63+
level: 'info',
64+
timestamp: expect.any(Number),
65+
event_id: expect.stringMatching(/\w{32}/),
66+
environment: 'production',
67+
tags: {},
68+
sdk: {
69+
integrations: expect.arrayContaining(['Feedback']),
70+
version: expect.any(String),
71+
name: 'sentry.javascript.browser',
72+
packages: expect.anything(),
73+
},
74+
request: {
75+
url: `${TEST_HOST}/index.html`,
76+
headers: {
77+
'User-Agent': expect.stringContaining(''),
78+
},
79+
},
80+
platform: 'javascript',
81+
});
82+
});

‎dev-packages/browser-integration-tests/suites/feedback/logger/init.js

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,19 @@
1+
import * as Sentry from '@sentry/browser';
2+
// Import this separately so that generatePlugin can handle it for CDN scenarios
3+
import { feedbackIntegration } from '@sentry/browser';
4+
5+
const feedback = feedbackIntegration({
6+
autoInject: false,
7+
});
8+
9+
window.Sentry = Sentry;
10+
window.feedback = feedback;
11+
12+
Sentry.init({
13+
dsn: 'https://[email protected]/1337',
14+
debug: true,
15+
integrations: [feedback],
16+
});
17+
18+
// This should log an error!
19+
feedback.attachTo('#does-not-exist');

‎dev-packages/browser-integration-tests/suites/feedback/logger/test.ts

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,36 @@
1+
import { expect } from '@playwright/test';
2+
3+
import { sentryTest } from '../../../utils/fixtures';
4+
import { shouldSkipFeedbackTest } from '../../../utils/helpers';
5+
6+
/**
7+
* This test is mostly relevant for ensuring that the logger works in all combinations of CDN bundles.
8+
* Even if feedback is included via the CDN, this test ensures that the logger is working correctly.
9+
*/
10+
sentryTest('should log error correctly', async ({ getLocalTestUrl, page }) => {
11+
// In minified bundles we do not have logger messages, so we skip the test
12+
if (shouldSkipFeedbackTest() || (process.env.PW_BUNDLE || '').includes('_min')) {
13+
sentryTest.skip();
14+
}
15+
16+
const messages: string[] = [];
17+
18+
page.on('console', message => {
19+
messages.push(message.text());
20+
});
21+
22+
await page.route('https://dsn.ingest.sentry.io/**/*', route => {
23+
return route.fulfill({
24+
status: 200,
25+
contentType: 'application/json',
26+
body: JSON.stringify({ id: 'test-id' }),
27+
});
28+
});
29+
30+
const url = await getLocalTestUrl({ testDir: __dirname });
31+
32+
await page.goto(url);
33+
34+
expect(messages).toContain('Sentry Logger [log]: Integration installed: Feedback');
35+
expect(messages).toContain('Sentry Logger [error]: [Feedback] Unable to attach to target element');
36+
});

‎dev-packages/browser-integration-tests/suites/integrations/httpclient/httpClientIntegration/init.js

Lines changed: 0 additions & 11 deletions
This file was deleted.

‎dev-packages/browser-integration-tests/suites/integrations/httpclient/httpClientIntegration/subject.js

Lines changed: 0 additions & 8 deletions
This file was deleted.

‎dev-packages/browser-integration-tests/suites/integrations/httpclient/httpClientIntegration/test.ts

Lines changed: 0 additions & 64 deletions
This file was deleted.

‎dev-packages/browser-integration-tests/suites/replay/logger/init.js

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,18 @@
1+
import * as Sentry from '@sentry/browser';
2+
3+
window.Sentry = Sentry;
4+
window.Replay = Sentry.replayIntegration({
5+
flushMinDelay: 200,
6+
flushMaxDelay: 200,
7+
minReplayDuration: 0,
8+
});
9+
10+
Sentry.init({
11+
dsn: 'https://[email protected]/1337',
12+
sampleRate: 0,
13+
replaysSessionSampleRate: 1.0,
14+
replaysOnErrorSampleRate: 0.0,
15+
debug: true,
16+
17+
integrations: [window.Replay],
18+
});

‎dev-packages/browser-integration-tests/suites/replay/logger/test.ts

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,36 @@
1+
import { expect } from '@playwright/test';
2+
3+
import { sentryTest } from '../../../utils/fixtures';
4+
import { shouldSkipReplayTest, waitForReplayRequest } from '../../../utils/replayHelpers';
5+
6+
sentryTest('should output logger messages', async ({ getLocalTestPath, page }) => {
7+
// In minified bundles we do not have logger messages, so we skip the test
8+
if (shouldSkipReplayTest() || (process.env.PW_BUNDLE || '').includes('_min')) {
9+
sentryTest.skip();
10+
}
11+
12+
const messages: string[] = [];
13+
14+
page.on('console', message => {
15+
messages.push(message.text());
16+
});
17+
18+
await page.route('https://dsn.ingest.sentry.io/**/*', route => {
19+
return route.fulfill({
20+
status: 200,
21+
contentType: 'application/json',
22+
body: JSON.stringify({ id: 'test-id' }),
23+
});
24+
});
25+
26+
const reqPromise0 = waitForReplayRequest(page, 0);
27+
28+
const url = await getLocalTestPath({ testDir: __dirname });
29+
30+
await Promise.all([page.goto(url), reqPromise0]);
31+
32+
expect(messages).toContain('Sentry Logger [log]: Integration installed: Replay');
33+
expect(messages).toContain('Sentry Logger [info]: [Replay] Creating new session');
34+
expect(messages).toContain('Sentry Logger [info]: [Replay] Starting replay in session mode');
35+
expect(messages).toContain('Sentry Logger [info]: [Replay] Using compression worker');
36+
});

‎dev-packages/browser-integration-tests/utils/generatePlugin.ts

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,6 @@ const useLoader = bundleKey.startsWith('loader');
3030
const IMPORTED_INTEGRATION_CDN_BUNDLE_PATHS: Record<string, string> = {
3131
httpClientIntegration: 'httpclient',
3232
captureConsoleIntegration: 'captureconsole',
33-
CaptureConsole: 'captureconsole',
3433
debugIntegration: 'debug',
3534
rewriteFramesIntegration: 'rewriteframes',
3635
contextLinesIntegration: 'contextlines',

‎packages/utils/src/logger.ts

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
import type { ConsoleLevel } from '@sentry/types';
22

33
import { DEBUG_BUILD } from './debug-build';
4-
import { GLOBAL_OBJ } from './worldwide';
4+
import { GLOBAL_OBJ, getGlobalSingleton } from './worldwide';
55

66
/** Prefix for logging strings */
77
const PREFIX = 'Sentry Logger ';
@@ -97,4 +97,8 @@ function makeLogger(): Logger {
9797
return logger as Logger;
9898
}
9999

100-
export const logger = makeLogger();
100+
/**
101+
* This is a logger singleton which either logs things or no-ops if logging is not enabled.
102+
* The logger is a singleton on the carrier, to ensure that a consistent logger is used throughout the SDK.
103+
*/
104+
export const logger = getGlobalSingleton('logger', makeLogger);

‎packages/utils/src/worldwide.ts

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@
1515
import type { Client, MetricsAggregator, Scope } from '@sentry/types';
1616

1717
import type { SdkSource } from './env';
18+
import type { logger } from './logger';
1819
import { SDK_VERSION } from './version';
1920

2021
interface SentryCarrier {
@@ -25,6 +26,7 @@ interface SentryCarrier {
2526
defaultIsolationScope?: Scope;
2627
defaultCurrentScope?: Scope;
2728
globalMetricsAggregators?: WeakMap<Client, MetricsAggregator> | undefined;
29+
logger?: typeof logger;
2830

2931
/** Overwrites TextEncoder used in `@sentry/utils`, need for `[email protected]` and older */
3032
encodePolyfill?: (input: string) => Uint8Array;

0 commit comments

Comments
 (0)