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

Make logging of validator-related failures more explicit #239

Merged
merged 1 commit into from
Jan 26, 2022

Conversation

eranrund
Copy link
Contributor

Motivation

I ran into this log message today:

 "details": "Error building transaction: Error generating FogPubkeyResolver Error fetching fog reports for [Uri { url: Url { scheme: \"fog\", cannot_be_a_base: false, username: \"\", password: None, host: Some(Domain(\"fog.prod.mobilecoinww.com\")), port: None, path: \"/\", query: None, fragment: None }, host: \"fog.prod.mobilecoinww.com\", port: 443, use_tls: true, username: \"\", password: \"\", _scheme: PhantomData }]: GRPC: RpcFailure: 14-UNAVAILABLE failed to connect to all addresses"

Initially it appeared to me as if fog.prod.mobilecoinww.com was not responding, but in reality it was the validator node that was not responding. This confusion will make future debugging more difficult, so I am proposing to make some logging more verbose.

In this PR

  • Making some failure cases log more verbosely

@eranrund eranrund changed the base branch from main to develop January 25, 2022 21:41
@@ -57,6 +58,7 @@ impl ValidatorConnection {
uri: uri.clone(),
validator_api_client,
blockchain_api_client,
logger,
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it make sense to add context to this logger so that it says that it's for the validator rather than adding "validator" to all the logged warnings/errors?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In my experience the context is not great for immediately communicating information to users, since its usually the message that gets looked at and not the custom fields.

@eranrund eranrund merged commit 166c31c into develop Jan 26, 2022
@eranrund eranrund deleted the eran/lvn-error-improvements branch January 26, 2022 18:55
briancorbin added a commit that referenced this pull request Feb 9, 2022
* make validator failure logs more verbose to ease debugging (#239)

* Fix a bug in receipt status endpoint. (#233)

* Uprev MobileCoin Library (#242)

* Update TXO documentation (#241)

* Update builder and base image references (#244)

* Feature/sync performance (#245)

* change tombstone block default to 10 (#246)

* adding max limit to some API endpoints (#248)
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