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 construction and usage in aggregator #2002

Merged
merged 12 commits into from
Oct 14, 2024

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

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

mithril-aggregator/src/dependency_injection/builder.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 🚀

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

4 participants