yarpc / yarpc-go

A message passing platform for Go
MIT License
401 stars 101 forks source link

Expose error message if the error is yarpcerror and has its own message #2218

Open unscrew opened 1 year ago

unscrew commented 1 year ago
CLAassistant commented 1 year ago

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.


Chloe Kim seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

uberesch commented 1 year ago

I like the inclusion of the original error but I don't like that we're combining it with the message field. It makes indexing a little harder. I think I should be able to filter whether the errors are inbound/outbound and inspect the cause separately. I generally expect log message fields to be unique string literals (unique for the line of code that logs the message) and for details to be decorated in separate fields.

uberesch commented 1 year ago

We currently have fatal crash logs that state A crash stack trace was detected; see 'stackTrace' field for crash details., you can then skim that field if you add it to the list of rendered fields. I still think that message fields should be string literals that map 1:1 to a unique line of code. Not going to object heavily to this change I just think there is merit to not having dynamic messages.

unscrew commented 1 year ago

@uberesch I don't think a couple of stings attached to the base string will make the log heavy compared to the other fields in the same log. Some even print out the entire object (I suggest not doing it). Exposing the meaningful/actionable original error message will help devs debug and monitor.

Also, it doesn't seem to be an expected behavior muting the original error message from yarpc library. Engineers always check the error messages and use this field to cluster the errors. Still, they can't do it all together since yarpc is exposing its own error message instead of the original error message to the error_message field.

This is not my opinion that the dominant error message Error handling inbound request. and Error making outbound call lowers the visibility of the monitoring tool. My team and our sister teams have had this complaint for a long time. I think many other teams would have similar feedback. Do you work at Uber? What's your uber unix name? I'll share the internal doc I'm working on.