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

fix: fix log in dispatcher code #3183

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from
Open

Conversation

samika98
Copy link
Contributor

@samika98 samika98 commented Mar 13, 2024

Description

Added more verbose logging to give more detail about what's going on.
These are the current logs : [2024-03-13T08:05:53.024Z] ERROR: Error while storing commit to IPFS: HTTPError: Not Found.

@samika98 samika98 requested review from stbrody and nathanielc March 13, 2024 16:24
@@ -367,9 +367,10 @@ export class Dispatcher {
Metrics.count(COMMITS_STORED, 1)
return cid
} catch (e) {
this._logger.err(`Error while storing commit to IPFS: ${e}`)
const errorMessage = `Error in repository.ts.storeInitEvent: Error - ${e} while storing commit to ipfs: ${e.message} | recon.mode: ${this.recon.enabled}`
this._logger.err(errorMessage)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we capture the backtrace here so we can see which code path the dispatcher was invoked from?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you log an Error instance directly it will log the stack trace automatically I believe

@@ -367,9 +367,10 @@ export class Dispatcher {
Metrics.count(COMMITS_STORED, 1)
return cid
} catch (e) {
this._logger.err(`Error while storing commit to IPFS: ${e}`)
const errorMessage = `Error in repository.ts.storeInitEvent: Error - ${e} while storing commit to ipfs: ${e.message} | recon.mode: ${this.recon.enabled}`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this meant to say we are in dispatcher.ts, not repository.ts.

But to be honest I don't really love putting the file name into the error message. If the message is descriptive and unique enough, it should be easy to grep for to find what file it is in. Putting the file name into the message feels brittle in case the code is ever refactored, and also kind like leaking information that isn't relevant to our users. I'd be okay with a stack trace though as that at least will update if the message moves and feels like a more standard thing devs know how to use.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also this doesn't really feel like the right place to log the recon mode either. I think global configuration like that should be logged once at startup, rather than every time there is an error.

@@ -367,9 +367,10 @@ export class Dispatcher {
Metrics.count(COMMITS_STORED, 1)
return cid
} catch (e) {
this._logger.err(`Error while storing commit to IPFS: ${e}`)
const errorMessage = `Error in repository.ts.storeInitEvent: Error - ${e} while storing commit to ipfs: ${e.message} | recon.mode: ${this.recon.enabled}`
this._logger.err(errorMessage)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you log an Error instance directly it will log the stack trace automatically I believe

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants