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

Change LogContext to attach an object #991

Open
arv opened this issue May 3, 2022 · 3 comments
Open

Change LogContext to attach an object #991

arv opened this issue May 3, 2022 · 3 comments
Labels
Future Something we want to fix but is not blocking next release

Comments

@arv
Copy link
Contributor

arv commented May 3, 2022

We have an internal class called LogContext. It's implementation is at https://github.com/rocicorp/logger/blob/main/src/logger.ts#L145

Basic usage is:

const lc = new LogContext().addContext('a', 'b');
lc?.info('c', 'd');
// roughly same as 
console.info('a=b', 'c', 'd');

This might not be too bad if it wasn't that our context always starts with the clientID which is ephemeral. For tools like Sentry that means that it is impossible to group the log lines.

An alternative approach would be to attach the context as a "JSON object". Then we would get something like:

const lc = new LogContext().addContext('a', 'b');
lc?.info('c', 'd');
// roughly same as 
console.info('c', 'd', {a: 'b'});

Then sentry would display this something along the lines of:

image

@phritz
Copy link
Contributor

phritz commented May 5, 2022

Yes this seems sensible, when it comes time to implement I'd like to talk about what goes into the message vs the attached extra context. There are a couple of ways we could go and it has bearing on how we log server-side, because we use LogContext ubiquitously and have have a few requirements on the server that are less relevant on the client.

@aboodman aboodman added good first issue P0 It's on fire and removed good first issue P0 It's on fire labels May 6, 2022
@aboodman
Copy link
Contributor

aboodman commented Jul 6, 2022

I was looking at this and a few questions (trying to setup Sentry myself to experiment):

  1. Can it really be true that it's impossible in Sentry to group by something other than the leading parameter?? It's quite pleasant/easy to read for me with clientID at the head of the line when displayed textually.
  2. Is it just the order of the log line that matters? If the context was at the end, textually, would that help?

@KeKs0r
Copy link

KeKs0r commented Jul 26, 2022

@aboodman Sentry can be configured how to group issues. But it is a more sophisticated effort and I think having sane grouping by default would be beneficial.

  1. I think just the log message itself is a very strong parameter on whether something is the same error or not.

The way I solved this now, is by wrapping error level logs into errors and parsing the log info into an object in order to attach it to the error:
https://gist.github.com/KeKs0r/ece9a454561bb864f806549d3b8e715c

@arv arv added Future Something we want to fix but is not blocking next release and removed User Reported labels Oct 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Future Something we want to fix but is not blocking next release
Projects
None yet
Development

No branches or pull requests

4 participants