Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

e2e test for background error telemetry #8717

Merged
merged 7 commits into from
Jun 28, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
160 changes: 134 additions & 26 deletions end-to-end-tests/tests/telemetry/errors.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,54 +26,68 @@ async function waitForBackgroundPageRequest(
return offscreenPage?.waitForRequest(errorServiceEndpoint);
}

async function getSentErrors(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
async function getSentErrors(
async function getForwardedErrors(

or

Suggested change
async function getSentErrors(
async function getErrorsFromRequest(

I don't know about anyone else, but my brain lumps "getSent" together instead of "sentErrors" together

extensionId: string,
context: BrowserContext,
errorServiceEndpoint: "https://browser-intake-datadoghq.com/api/v2/*",
) {
// TODO: due to Datadog SDK implementation, it will take ~30 seconds for the
// request to be sent. We should figure out a way to induce the request to be sent sooner.
// See this datadog support request: https://help.datadoghq.com/hc/en-us/requests/1754158
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❤️ thanks again to reaching out to support

const request = await waitForBackgroundPageRequest(
context,
extensionId,
errorServiceEndpoint,
);

return request
?.postData()
?.split("\n")
.map((log) => JSON.parse(log));
}

test.use({
additionalRequiredEnvVariables: [
"DATADOG_CLIENT_TOKEN",
"DEV_EVENT_TELEMETRY",
],
});

test("can report application error to telemetry service", async ({
test("can report extension console error to telemetry service", async ({
fungairino marked this conversation as resolved.
Show resolved Hide resolved
page,
context,
extensionId,
}) => {
const errorServiceEndpoint = "https://browser-intake-datadoghq.com/api/v2/*";

await context.route(
"https://app.pixiebrix.com/api/extensions/",
async (route) => {
await route.fulfill({
status: 200,
// Returning a bad response to trigger an error
body: JSON.stringify([{}]),
});
},
);
await test.step("Mock the extensions endpoint to return a bad response, and mock errorService calls", async () => {
await context.route(
"https://app.pixiebrix.com/api/extensions/",
async (route) => {
await route.fulfill({
status: 200,
body: JSON.stringify([{}]),
});
},
);

await context.route(errorServiceEndpoint, async (route) =>
route.fulfill({
status: 202,
}),
);
await context.route(errorServiceEndpoint, async (route) =>
route.fulfill({
status: 202,
}),
);
});

await page.goto(getBaseExtensionConsoleUrl(extensionId));
await expect(page.getByText("Something went wrong.")).toBeVisible();

// TODO: due to Datadog SDK implementation, it will take ~30 seconds for the
// request to be sent. We should figure out a way to induce the request to be sent sooner.
const request = await waitForBackgroundPageRequest(
context,
const sentErrors = await getSentErrors(
extensionId,
context,
errorServiceEndpoint,
);

const errorLogsJson = request
?.postData()
?.split("\n")
.map((log) => JSON.parse(log));

expect(errorLogsJson).toContainEqual(
expect(sentErrors).toContainEqual(
expect.objectContaining({
service: "pixiebrix-browser-extension",
manifestVersion: 3,
Expand All @@ -82,6 +96,100 @@ test("can report application error to telemetry service", async ({
message: expect.any(String),
kind: expect.any(String),
}),
// Stack and message are duplicated
Copy link
Collaborator

@mnholtz mnholtz Jun 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure what the intended goal with this one is. Is this a bug? Does message == stack? If so, are we missing the stack or the message?

I could at least see a comment like this being better suited for where we add the error message in the first place as opposed to a test, e.g.

const errorMessage = getErrorMessage(error);

But at best I'd like to clarify if this is a problem or not, and create a ticket if so.

stack: expect.any(String),
message: expect.any(String),
connectionType: expect.any(String),
date: expect.any(Number),
extensionVersion: expect.any(String),
name: expect.any(String),
origin: "logger",
pageName: "options",
referrer: "",
runtimeId: extensionId,
session_id: expect.any(String),
status: "error",
url: `chrome-extension://${extensionId}/options.html#/`,
usr: {
email: "[email protected]",
id: "3f7ac0b4-5029-442c-b537-5de9f1dfdfd9",
organizationId: "47f616c5-81e3-4edb-ba44-ed5dd4a78c08",
},
view: {
referrer: "",
url: `chrome-extension://${extensionId}/offscreen.html`,
},
}),
);
});

test("can report a service worker error to telemetry service", async ({
page,
context,
extensionId,
}) => {
const errorServiceEndpoint = "https://browser-intake-datadoghq.com/api/v2/*";
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: opportunity to extract errorServiceEndpoint


await test.step("Mock the registry endpoint to return a bad response, and mock errorService calls", async () => {
await context.route(
"https://app.pixiebrix.com/api/registry/bricks/",
Copy link
Collaborator

@mnholtz mnholtz Jun 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would extract this endpoint into a constant called something like endpointCalledFromServiceWorker to clarify what makes this test different from the first, and how this test covers the service worker case.

async (route) => {
await route.fulfill({
status: 500,
body: "I'm not json!",
});
},
);

await context.route(errorServiceEndpoint, async (route) =>
route.fulfill({
status: 202,
}),
);
});

await page.goto(getBaseExtensionConsoleUrl(extensionId));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be too early to extract at 2x usages, but maybe if we end up adding a third test with this shape, we should extract this logic into something reusable.

await expect(page.getByText("An error occurred")).toBeVisible();

const sentErrors = await getSentErrors(
extensionId,
context,
errorServiceEndpoint,
);

expect(sentErrors).toContainEqual(
expect.objectContaining({
code: "ERR_BAD_RESPONSE",
code_version: expect.any(String),
connectionType: "4g",
date: expect.any(Number),
error: expect.objectContaining({
kind: "AxiosError",
message: expect.any(String),
stack: expect.any(String),
}),
extensionVersion: expect.any(String),
manifestVersion: 3,
message: expect.any(String),
name: "AxiosError",
origin: "logger",
pageName: "background",
referrer: "",
runtimeId: extensionId,
service: "pixiebrix-browser-extension",
session_id: expect.any(String),
stack: expect.any(String),
status: "error",
url: "https://app.pixiebrix.com/api/registry/bricks/",
usr: {
email: "[email protected]",
id: "3f7ac0b4-5029-442c-b537-5de9f1dfdfd9",
organizationId: "47f616c5-81e3-4edb-ba44-ed5dd4a78c08",
},
view: {
referrer: "",
url: `chrome-extension://${extensionId}/offscreen.html`,
},
}),
);
});
6 changes: 2 additions & 4 deletions src/registry/packageRegistry.ts
Original file line number Diff line number Diff line change
Expand Up @@ -125,15 +125,13 @@ function latestVersion(
* Replace the local database with the packages from the registry.
*
* Memoized to avoid multiple network requests across tabs.
* Consecutive calls are not an issue since the first call is unauthenticated and the
* second is after the user authenticates, because the extension reloads on linking
*/
export const syncPackages = memoizeUntilSettled(async () => {
// The endpoint doesn't return the updated_at timestamp. So use the current local time as our timestamp.
const timestamp = new Date();

// XXX: we currently don't have to worry about consecutive calls where the first call is unauthenticated and the
// second is after the user authenticates, because the extension reloads on linking
const client = await getApiClient();
// In the future, use the paginated endpoint?
const { data } = await client.get<RegistryPackage[]>("/api/registry/bricks/");

const packages = data.map((x) => ({
Expand Down
Loading