[backup] Improve logging
ClosedPublic
Actions

Authored by michal on Dec 22 2023, 7:35 AM.

Details

Reviewers

Commits

rCOMM720abacad0cd: [backup] Improve logging

Summary

Improve logging in the backup service. Cleaned up the instrument macros.

Depends on D10428

Test Plan

Run tests, check that logs are correctly displayed.

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

services/backup/src/http/handlers/backup.rs
32 ↗	(On Diff #34988)	This was moved because otherwise it didn't have the `backup_id` field (as it's filled in later in the `current().record` call). If there are any errors before this line they should still be displayed in the span (but without the `backup_id` field).
shared/comm-lib/src/auth/types.rs
113–116 ↗	(On Diff #34988)	I'm only logging `user_id` and `device_id` here and skipping the access token. I think that's fine but I want to make sure? (cc @varun, @ashoat)

michal requested review of this revision.Dec 22 2023, 7:53 AM

shared/comm-lib/src/auth/types.rs
113–116 ↗	(On Diff #34988)	Definitely we should skip the access token. I wonder a little bit about logging `user_id` and `device_id`... is it necessary? I'm wondering if we're basically creating an access log of when people use Comm, which is metadata that could be used to compromise privacy

shared/comm-lib/src/auth/types.rs
113–116 ↗	(On Diff #34988)	When I was working on the initial version of the backup service with @kamil it was already pretty hard to debug when something went wrong (e.g. mapping a failed backup upload on a client to the service logs) and it will be even worse with real usage. The backup stored in db already keeps the `user_id` and creation timestamp. But I understand the security concern and it's not strictly necessary so I can remove it if it's risky.

shared/comm-lib/src/auth/types.rs
113–116 ↗	(On Diff #34988)	Let's remove it for now, and we can bring it back temporarily if we really think it's necessary at some point

Requesting changes to remove sensitive fields for now

shared/comm-lib/src/auth/types.rs
113–116 ↗	(On Diff #34988)	If it was included only for debug purposes (no info/warn/error logs), then it could stay, here this is used as "instrument" field so it will be displayed in all logs and we don't want it. For debugging purposes, this is already implemented in `#[derive(Debug)]` trait. On the other hand, I strongly agree that having such logs greatly improves debugging experience, especially on staging/prod, Maybe we could use sth like "hash key" for this struct, so in logs we're at least able to distinguish which logs belong to given request/backup/owner. For instance, some HTTP loggers include random `request_id` which allow to track logs when there are multiple simultaneous requests.