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

Enhance logs in mithril-signer and mithril-persistence #1992

Merged
merged 8 commits into from
Oct 10, 2024

Conversation

Alenar
Copy link
Collaborator

@Alenar Alenar commented Oct 9, 2024

Content

This PR refactor the log usage in mithril-signer and mithril-persistence:

  • A dedicated child logger is used for each service
  • Each child logger is tagged with its service name in order to identify quickly a log source without the need of a prefix in the log message.
  • Remove slog_scope to enforce the usage of a name tagged child logger (only kept in the integration tests).
  • Change the name property in all logs from the default slog-rs to mithril-signer.
  • Move away from the log message into a property huge structure (ie: some logs joined errors into their message, those errors have been moved to a "error" property).

Also this PR enhance errors issued by the signer AggregatorClient when there's an http error:

  • the status code canonical name (ie: "bad request" for a 400) is prepend to the error message. This add some context when there's no body in the http response (previously there were only the back trace).
  • if a json body is available in the http response, it tries to deserialize it as either a ClientError or ServerError (returned by the aggregator with 4xx and 5xx errors respectively). There's fallback in case of unknown format.

Also this PR fix an issue when registering a signature: if the aggregator returned a 202 (Accepted) status code it was handled as an error, making the state machine fail its cycle.

Pre-submit checklist

  • Branch
    • Tests are provided (if possible)
    • Crates versions are updated (if relevant)
    • CHANGELOG file is updated (if relevant)
    • Commit sequence broadly makes sense
    • Key commits have useful messages
  • PR
    • No clippy warnings in the CI
    • Self-reviewed the diff
    • Useful pull request description
    • Reviewer requested

Issue(s)

Relates to #1981

@Alenar Alenar requested review from sfauvel, jpraynaud and dlachaume and removed request for sfauvel and jpraynaud October 9, 2024 14:09
Copy link

github-actions bot commented Oct 9, 2024

Test Results

    4 files  ± 0     54 suites  ±0   9m 33s ⏱️ +16s
1 351 tests +13  1 351 ✅ +13  0 💤 ±0  0 ❌ ±0 
1 559 runs  +13  1 559 ✅ +13  0 💤 ±0  0 ❌ ±0 

Results for commit ca7789a. ± Comparison against base commit 03aa794.

♻️ This comment has been updated with latest results.

@Alenar Alenar force-pushed the djo/1981/enhance_logs_in_signer branch from 871be9c to 02cd14d Compare October 9, 2024 14:48
@Alenar Alenar temporarily deployed to testing-preview October 9, 2024 14:56 — with GitHub Actions Inactive
@Alenar Alenar temporarily deployed to testing-sanchonet October 9, 2024 14:56 — with GitHub Actions Inactive
@Alenar Alenar force-pushed the djo/1981/enhance_logs_in_signer branch from 02cd14d to 25769a2 Compare October 9, 2024 15:06
@Alenar Alenar temporarily deployed to testing-preview October 9, 2024 15:14 — with GitHub Actions Inactive
@Alenar Alenar temporarily deployed to testing-sanchonet October 9, 2024 15:14 — with GitHub Actions Inactive
Copy link
Collaborator

@sfauvel sfauvel left a comment

Choose a reason for hiding this comment

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

LGTM

mithril-signer/src/runtime/error.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@dlachaume dlachaume left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

Copy link
Member

@jpraynaud jpraynaud left a comment

Choose a reason for hiding this comment

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

LGTM 👍

Each service now create a child logger that's tagged with its name so we
can know more easily the source of a log.

This allow to remove `slog_scope` from the main dependencies (its still
used by the integration tests).
Since now each logs are tagged with their source component.
+ add some logs in the connection builder
+ remove unneeded 'slog-*' dependencies
And instead add them to a property. This allow to keep the main log
messages leans while still providing detailed errors info.
* Always include the status code text in the issued error, this should add
  more context when the response text is empty.
* When the response contains json: try to parse ideally as a `ClientError`
  or `ServerError` to use their properties, if of an unknown type it will
  output all the json key/value pairs as a fallback.
@Alenar Alenar force-pushed the djo/1981/enhance_logs_in_signer branch from 25769a2 to be2657a Compare October 10, 2024 10:33
* mithril-persistence from `0.2.27` to `0.2.28`
* mithril-signer from `0.2.195` to `0.2.196`
@Alenar Alenar temporarily deployed to testing-preview October 10, 2024 11:37 — with GitHub Actions Inactive
@Alenar Alenar temporarily deployed to testing-sanchonet October 10, 2024 11:37 — with GitHub Actions Inactive
@Alenar Alenar merged commit b9b227f into main Oct 10, 2024
48 checks passed
@Alenar Alenar deleted the djo/1981/enhance_logs_in_signer branch October 10, 2024 11:56
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.

4 participants