#9221: Attach db name and operation name to idb errors #9223

grahamlangford · 2024-10-01T20:51:25Z

What does this PR do?

Closes Add consistent idb operation execution and error handling across all idb databases #9221

Discussion

Still rethrows the errors, which means some errors will likely be double-reported
In logging.ts, we swallow some of the errors but not others
- @fungairino, what criteria did you use to determine which errors should be swallowed?

For more information on our expectations for the PR process, see the
code review principles doc

grahamlangford · 2024-10-01T20:53:02Z

src/telemetry/reportToApplicationErrorTelemetry.test.ts

Tests copied from logging.test.ts

grahamlangford · 2024-10-01T20:53:21Z

src/telemetry/reportToApplicationErrorTelemetry.ts

Extracted from logging.ts for reuse

codecov · 2024-10-01T20:58:48Z

Codecov Report

Attention: Patch coverage is 75.00000% with 26 lines in your changes missing coverage. Please review.

Project coverage is 74.81%. Comparing base (8318d74) to head (175653b).
Report is 328 commits behind head on main.

Files with missing lines	Patch %	Lines
src/telemetry/reportToApplicationErrorTelemetry.ts	72.09%	12 Missing ⚠️
src/telemetry/logging.ts	60.00%	6 Missing ⚠️
src/telemetry/trace.ts	58.33%	5 Missing ⚠️
src/registry/packageRegistry.ts	75.00%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #9223      +/-   ##
==========================================
+ Coverage   74.24%   74.81%   +0.56%     
==========================================
  Files        1332     1367      +35     
  Lines       40817    42139    +1322     
  Branches     7634     7879     +245     
==========================================
+ Hits        30306    31526    +1220     
- Misses      10511    10613     +102

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

github-actions · 2024-10-01T21:10:05Z

Playwright test results

124 passed
10 flaky
4 skipped

Details

Open report ↗︎
138 tests across 45 suites
33 minutes, 10 seconds
175653b
For more information on how to debug and view this report, see our readme

Flaky tests

chrome-setup › setup/affiliated.setup.ts › authenticate with affiliated user
chrome-setup › setup/affiliated.setup.ts › authenticate with affiliated user
chrome-setup › setup/unaffiliated.setup.ts › authenticate with unaffiliated user
chrome › tests/extensionConsole/activation.spec.ts › activating a mod when the quickbar shortcut is not configured
msedge › tests/extensionConsole/modsPage.spec.ts › can open mod in the workshop
msedge › tests/modLifecycle.spec.ts › create, run, package, and update mod
chrome › tests/pageEditor/addStarterBrick.spec.ts › Add starter brick to mod
msedge › tests/pageEditor/addStarterBrick.spec.ts › Add starter brick to mod
msedge › tests/pageEditor/saveMod.spec.ts › shows error notification when updating a public mod without incrementing the version
msedge › tests/runtime/sidebar/sidebarNavigation.spec.ts › sidebar mod panels are persistent during navigation

Skipped tests

chrome › tests/regressions/doNotCloseSidebarOnPageEditorSave.spec.ts › #8104: Do not automatically close the sidebar when saving in the Page Editor
msedge › tests/regressions/doNotCloseSidebarOnPageEditorSave.spec.ts › #8104: Do not automatically close the sidebar when saving in the Page Editor
chrome › tests/runtime/googleSheetsIntegration.spec.ts › can activate a google spreadsheet mod with config options
msedge › tests/runtime/googleSheetsIntegration.spec.ts › can activate a google spreadsheet mod with config options

fungairino · 2024-10-02T13:43:47Z

src/utils/idbUtils.ts

+// Rather than use reportError from @/telemetry/reportError, IDB errors are directly reported
+// to application error telemetry to avoid attempting to record the error in the idb log database.


This was the case with errors that got thrown when writing to the idb LOG database, but I think that we probably should call the normal reportError when writing to the other dbs so that if the error is related to a mod run it will get recorded in the idb log database.

To handle this we could consider defining an optional reportError param that defaults to using reportError and we can override that for the logging db operations.

I agree, but we would need to refactor the report functions. It's not worth the added effort right now. We're not catching the errors that we're using to make reportToApplicationErrorTelemetry calls, so the error will still be reported to the error service and the logging idb

fungairino · 2024-10-02T13:53:08Z

src/utils/idbUtils.ts

+      | ValueOf<typeof IDB_OPERATION.LOG>
+      | ValueOf<typeof IDB_OPERATION.TRACE>
+      | ValueOf<typeof IDB_OPERATION.PACKAGE_REGISTRY>;


nit: there is probably a more terse way to write this, but this is probably fine. Maybe consider defining a type for this.

Another way to write this:

ValueOf< | typeof IDB_OPERATION.LOG | typeof IDB_OPERATION.TRACE | typeof IDB_OPERATION.PACKAGE_REGISTRY >;

fungairino · 2024-10-02T13:55:16Z

src/utils/idbUtils.ts

+  PACKAGE_REGISTRY: "BRICK_REGISTRY",
+} as const;
+
+export const IDB_OPERATION = {


We might want to consider defining the common operations as helper methods in this file like count and recreateDB

I don't disagree. But I think we should do that as a separate PR if that's a path we want to go down

fungairino · 2024-10-02T13:57:16Z

src/utils/idbUtils.ts

+} as const;
+
+export const IDB_OPERATION = {
+  LOG: {


nit: you could make this refer to the const you defined above for stricter typing:

[DATABASE_NAME.LOG]: { ...

fungairino · 2024-10-02T14:07:33Z

In logging.ts, we swallow some of the errors but not others
@fungairino, what criteria did you use to determine which errors should be swallowed?

Some functions were used in features or returned a value in which case they were not swallowed. In cases were we returning void, and didn't care if the function was successful, we swallowed the error.

fungairino · 2024-10-02T17:17:53Z

end-to-end-tests/pageObjects/extensionConsole/modsPage.ts

@@ -64,7 +64,7 @@ export class ModsPage extends BasePageObject {
      .waitForEvent("requestfinished", {
        predicate: (request) =>
          request.url().includes(API_PATHS.REGISTRY_BRICKS),
-        timeout: 15_000,
+        timeout: 30_000,


Can we create a github issue for this and link it in an inline comment?

github-actions · 2024-10-02T17:18:00Z

No loom links were found in the first post. Please add one there if you'd like to it to appear on Slack.

Do not edit this comment manually.

grahamlangford added 4 commits October 1, 2024 14:33

refactor to extract shared logic from logging.ts

9b41caa

implement logging for trace db

1adf432

implement logging error catching for packageRegistry

14098a6

move tests

af3a072

grahamlangford self-assigned this Oct 1, 2024

grahamlangford added this to the 2.1.3 milestone Oct 1, 2024

grahamlangford requested review from twschiller, fungairino and mnholtz October 1, 2024 20:51

grahamlangford commented Oct 1, 2024

View reviewed changes

src/telemetry/reportToApplicationErrorTelemetry.test.ts Outdated

Copy link

Collaborator Author

grahamlangford Oct 1, 2024

Choose a reason for hiding this comment

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

Tests copied from logging.test.ts

grahamlangford commented Oct 1, 2024

View reviewed changes

twschiller added error-telemetry telemetry customer Required for a customer projct labels Oct 2, 2024

fungairino reviewed Oct 2, 2024

View reviewed changes

stricter typing for operation names

573c481

vercel bot deployed to Preview October 2, 2024 14:12 View deployment

grahamlangford added 2 commits October 2, 2024 09:15

further tighten typing

0a2da87

fix type errors

7d6fd00

vercel bot deployed to Preview October 2, 2024 14:40 View deployment

remove unused import

e0f5284

vercel bot deployed to Preview October 2, 2024 14:51 View deployment

increase timeout

b7ce00d

vercel bot deployed to Preview October 2, 2024 16:09 View deployment

raise timeout to 30s

175653b

vercel bot deployed to Preview October 2, 2024 16:49 View deployment

fungairino approved these changes Oct 2, 2024

View reviewed changes

fungairino reviewed Oct 2, 2024

View reviewed changes

grahamlangford merged commit 1279b4c into main Oct 2, 2024
26 checks passed

grahamlangford deleted the attach-db-name-and-operation-name-to-idb-errors branch October 2, 2024 17:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#9221: Attach db name and operation name to idb errors #9223

#9221: Attach db name and operation name to idb errors #9223

grahamlangford commented Oct 1, 2024

grahamlangford Oct 1, 2024

grahamlangford Oct 1, 2024

codecov bot commented Oct 1, 2024 •

edited

Loading

github-actions bot commented Oct 1, 2024 •

edited

Loading

fungairino Oct 2, 2024 •

edited

Loading

grahamlangford Oct 2, 2024

fungairino Oct 2, 2024

fungairino Oct 2, 2024

grahamlangford Oct 2, 2024

fungairino Oct 2, 2024

fungairino commented Oct 2, 2024

fungairino Oct 2, 2024 •

edited

Loading

github-actions bot commented Oct 2, 2024

		// Rather than use reportError from @/telemetry/reportError, IDB errors are directly reported
		// to application error telemetry to avoid attempting to record the error in the idb log database.

#9221: Attach db name and operation name to idb errors #9223

#9221: Attach db name and operation name to idb errors #9223

Conversation

grahamlangford commented Oct 1, 2024

What does this PR do?

Discussion

grahamlangford Oct 1, 2024

Choose a reason for hiding this comment

grahamlangford Oct 1, 2024

Choose a reason for hiding this comment

codecov bot commented Oct 1, 2024 • edited Loading

Codecov Report

github-actions bot commented Oct 1, 2024 • edited Loading

Playwright test results

Details

fungairino Oct 2, 2024 • edited Loading

Choose a reason for hiding this comment

grahamlangford Oct 2, 2024

Choose a reason for hiding this comment

fungairino Oct 2, 2024

Choose a reason for hiding this comment

fungairino Oct 2, 2024

Choose a reason for hiding this comment

grahamlangford Oct 2, 2024

Choose a reason for hiding this comment

fungairino Oct 2, 2024

Choose a reason for hiding this comment

fungairino commented Oct 2, 2024

fungairino Oct 2, 2024 • edited Loading

Choose a reason for hiding this comment

github-actions bot commented Oct 2, 2024

codecov bot commented Oct 1, 2024 •

edited

Loading

github-actions bot commented Oct 1, 2024 •

edited

Loading

fungairino Oct 2, 2024 •

edited

Loading

fungairino Oct 2, 2024 •

edited

Loading