Page MenuHomePhabricator
Differential D8614

Improve error logging for olm session creation failure
ClosedPublic
Actions

Authored by marcin on Jul 25 2023, 3:12 AM.
Tags
None
Referenced Files
F2894123: D8614.diff
Fri, Oct 4, 2:41 PM
Unknown Object (File)
Wed, Oct 2, 7:10 PM
Unknown Object (File)
Fri, Sep 20, 1:55 AM
Unknown Object (File)
Fri, Sep 20, 1:55 AM
Unknown Object (File)
Fri, Sep 20, 1:55 AM
Unknown Object (File)
Fri, Sep 20, 1:55 AM
Unknown Object (File)
Fri, Sep 20, 1:55 AM
Unknown Object (File)
Tue, Sep 10, 6:30 AM
View All 65 Files
Subscribers
None

Details

Reviewers
tomek
ashoat
Commits
rCOMMfa412e696c0b: Improve error logging for olm session creation failure
Summary

This differential is created in response to the following linear issue: https://linear.app/comm/issue/ENG-4451/improve-logging-for-olm-session-creation-failure. Olm session creation failure is a seriious concern and current error handling does
not provide meaningful information for debugging purposes.

Test Plan

Delete olm sessions table from the database. Try to log in from mobile app. Ensure that meaningful logs are printed to the console.

Diff Detail

Repository
rCOMM Comm
Branch
marcin/eng-4453
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

marcin created this revision.Jul 25 2023, 3:12 AM
Harbormaster returned this revision to the author for changes because remote builds failed.Jul 25 2023, 4:33 AM
Harbormaster failed remote builds in B21177: Diff 29010!
Harbormaster completed remote builds in B21177: Diff 29010.Jul 25 2023, 5:09 AM
marcin requested review of this revision.Jul 25 2023, 5:09 AM
tomek accepted this revision.Jul 25 2023, 6:17 AM
tomek added inline comments.
keyserver/src/creators/olm-session-creator.js
27 ↗(On Diff #29010)

We probably shouldn't log cookieID

This revision is now accepted and ready to land.Jul 25 2023, 6:17 AM
marcin added inline comments.Jul 25 2023, 9:40 AM
keyserver/src/creators/olm-session-creator.js
27 ↗(On Diff #29010)

Knowing the cookieID for which the failure occurred might be helpful for debugging purposes. Does it create security issues?

ashoat added inline comments.Jul 25 2023, 11:47 AM
keyserver/src/creators/olm-session-creator.js
27 ↗(On Diff #29010)
  1. Can you please keep all lines to a max of 80 chars?
  2. I think it's okay to log the cookie ID on the keyserver. Presumably the keyserver owner has access to this information anyways via MariaDB
  3. I wonder if relying on Node to render the exception might be better than getMessageForException, eg. console.warn('message', e)
marcin updated this revision to Diff 29057.Jul 26 2023, 7:14 AM

Address comments

Harbormaster completed remote builds in B21214: Diff 29057.Jul 26 2023, 7:32 AM
ashoat accepted this revision.Jul 27 2023, 6:22 PM
ashoat added inline comments.
keyserver/src/creators/olm-session-creator.js
27

Missing a space at the end

marcin updated this revision to Diff 29161.Jul 28 2023, 4:04 AM

Add missing space. Rebase before landing

marcin updated this revision to Diff 29162.Jul 28 2023, 4:08 AM

Rebase before landing

Harbormaster completed remote builds in B21293: Diff 29161.Jul 28 2023, 4:22 AM
Harbormaster completed remote builds in B21294: Diff 29162.Jul 28 2023, 4:33 AM
Closed by commit rCOMMfa412e696c0b: Improve error logging for olm session creation failure (authored by marcin). · Explain WhyJul 28 2023, 4:42 AM
This revision was automatically updated to reflect the committed changes.
marcin added a commit: rCOMMfa412e696c0b: Improve error logging for olm session creation failure.

Revision Contents
Changeset List

PathSize
keyserver/
src/
creators/
5 lines

Diff 29057

View Options

keyserver/src/creators/olm-session-creator.js

Loading...