Skip to content

Conversation

@tynes
Copy link
Contributor

@tynes tynes commented May 11, 2021

Description
This PR replaces all instances of log.error({ err }) in the codebase such that it logs the error message and stack. The stack should never be returned to the user via an HTTP request, only we should be able to see these messages

Additional context
Previously these would log as empty objects so they wouldn't be particularly helpful

@changeset-bot
Copy link

changeset-bot bot commented May 11, 2021

🦋 Changeset detected

Latest commit: 9b4847f

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 3 packages
Name Type
@eth-optimism/batch-submitter Patch
@eth-optimism/data-transport-layer Patch
@eth-optimism/message-relayer Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@tynes tynes requested a review from snario May 11, 2021 21:51
@snario
Copy link
Contributor

snario commented May 11, 2021

I'm still confused as to why we need to write the object explicitly like this. I added this PR to address it and I do recall seeing error traces appear in Sentry using it. So, perhaps it's an issue with pino-sentry not displaying the error in the log message but still correctly forwarding it to Sentry? I see this PR from the time I was working on The Graph and it seems like perhaps there is a log line length restriction?

Note that this is not about blocking this PR; just an attempt at demystifying this issue.

@codecov-commenter
Copy link

codecov-commenter commented May 11, 2021

Codecov Report

Merging #843 (5d86105) into develop (8d67991) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff            @@
##           develop     #843   +/-   ##
========================================
  Coverage    82.21%   82.21%           
========================================
  Files           48       48           
  Lines         1895     1895           
  Branches       303      303           
========================================
  Hits          1558     1558           
  Misses         337      337           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8d67991...5d86105. Read the comment docs.

@tynes tynes merged commit fa4898a into develop May 12, 2021
@tynes tynes deleted the fix/logging-errors branch May 12, 2021 06:08
@tynes
Copy link
Contributor Author

tynes commented May 12, 2021

I'm still confused as to why we need to write the object explicitly like this. I added this PR to address it and I do recall seeing error traces appear in Sentry using it. So, perhaps it's an issue with pino-sentry not displaying the error in the log message but still correctly forwarding it to Sentry? I see this PR from the time I was working on The Graph and it seems like perhaps there is a log line length restriction?

Note that this is not about blocking this PR; just an attempt at demystifying this issue.

I'm not quite sure either, although I do think its a syntax thing when it comes to using the shorthand with a more complex type than a literal/basic object. If the errors were appearing in sentry then perhaps I am wrong but I do feel like I have encountered this error before in other projects

InoMurko pushed a commit to omgnetwork/optimism that referenced this pull request May 25, 2021
* batch-submitter: log error explicitly

* data-transport-layer: log error explicitly

* message-relayer: log error explicitly

* chore: add changeset
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-op-batcher Area: op-batcher

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants