Skip to content

Conversation

@Alenar
Copy link
Collaborator

@Alenar Alenar commented Oct 11, 2024

Content

This PR refactor the log usage in mithril-aggregator:

  • 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.
  • 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.

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

Comments

As this PR is already quite big, a second PR will come that will pass over all logs in the aggregator to move away from the log message huge structure into properties, to remove prefixes in message as now all logs are tagged with their source, and to enforce a common style & structure.

Issue(s)

Relates to #1981

@github-actions
Copy link

github-actions bot commented Oct 11, 2024

Test Results

    4 files  ±0     54 suites  ±0   9m 25s ⏱️ ±0s
1 352 tests +1  1 352 ✅ +1  0 💤 ±0  0 ❌ ±0 
1 560 runs  +1  1 560 ✅ +1  0 💤 ±0  0 ❌ ±0 

Results for commit 01bf63f. ± Comparison against base commit 0bc09f4.

♻️ This comment has been updated with latest results.

Use full path instead of needing a use gated by an `cfg(test)` that add
noises with the other imports.
To create child logger when there's no component to hold them (such
as the http server in aggregator).
* Tagged with the component name.
* Existing logs are still using `slog_scope` but this prepare its
  removal.
… http server

To tag the logs with a source so they may be easily filtered.
… services & tools

To tag the logs with a source so they may be easily filtered.
@Alenar Alenar force-pushed the djo/1981/enhance_logs_in_aggregator branch from a9f8587 to 24be951 Compare October 11, 2024 17:33
@Alenar Alenar temporarily deployed to testing-preview October 11, 2024 17:44 — with GitHub Actions Inactive
@Alenar Alenar temporarily deployed to testing-sanchonet October 11, 2024 17:44 — with GitHub Actions Inactive
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 👍

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.

Good work
Just a doubt about a function name

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 🚀

@Alenar Alenar force-pushed the djo/1981/enhance_logs_in_aggregator branch from 24be951 to 01bf63f Compare October 14, 2024 10:40
@Alenar Alenar merged commit 28dd7ae into main Oct 14, 2024
36 of 38 checks passed
@Alenar Alenar deleted the djo/1981/enhance_logs_in_aggregator branch October 14, 2024 12:43
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.

5 participants