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.

Sign up for GitHub

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

Jump to bottom

Rework & harmonize aggregator log messages #2009

Merged
merged 11 commits into from
Oct 15, 2024
Merged

Rework & harmonize aggregator log messages #2009

merged 11 commits into from
Oct 15, 2024

Conversation

Alenar
Copy link
Collaborator

@Alenar Alenar commented Oct 14, 2024 •
edited
Loading

Content

This PR:

  • Harmonize all "function enter" logs message to be in the form of >> function_name.
    • Excepted in the http server where they are in the form of HTTP_VERB /path/to/endpoint.
  • Remove all prefixes previously used to help find the log of a component since they logs are tagged with their source component since Enhance logs construction and usage in aggregator #2002.
  • Capitalize the first letters of all "functional" logs.
  • 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).

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

@github-actions GitHub Actions
Copy link

github-actions bot commented Oct 14, 2024 •
edited
Loading

Test Results

    4 files  ±0     54 suites  ±0   9m 26s ⏱️ +7s
1 358 tests +6  1 358 ✅ +6  0 💤 ±0  0 ❌ ±0 
1 566 runs  +6  1 566 ✅ +6  0 💤 ±0  0 ❌ ±0 

Results for commit 20156b9. ± Comparison against base commit 8fbaaee.

♻️ This comment has been updated with latest results.

jpraynaud
jpraynaud approved these changes Oct 15, 2024
View reviewed changes
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 🚀

mithril-aggregator/src/tools/signer_importer.rs Outdated Show resolved Hide resolved
dlachaume
dlachaume approved these changes Oct 15, 2024
View reviewed changes
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 🚀
I've added some additional data to the input logs of the aggregator runner functions. It’s just a suggestion, i'm not 100% sure if it brings real value without cluttering the logs. Also included a few rephrasing suggestions and typo in the HTTP routes logs.

mithril-aggregator/src/http_server/routes/artifact_routes/cardano_stake_distribution.rs Outdated Show resolved Hide resolved
mithril-aggregator/src/http_server/routes/artifact_routes/snapshot.rs Outdated Show resolved Hide resolved
mithril-aggregator/src/runtime/runner.rs Show resolved Hide resolved
mithril-aggregator/src/runtime/runner.rs Outdated Show resolved Hide resolved
mithril-aggregator/src/runtime/runner.rs Outdated Show resolved Hide resolved
mithril-aggregator/src/runtime/runner.rs Outdated Show resolved Hide resolved
mithril-aggregator/src/services/cardano_transactions_importer.rs Outdated Show resolved Hide resolved
mithril-aggregator/src/services/cardano_transactions_importer.rs Outdated Show resolved Hide resolved
sfauvel
sfauvel approved these changes Oct 15, 2024
View reviewed changes
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

@Alenar
style(aggregator): remove prefixes used in logs
3fcbfd5 
Since now each logs are tagged with their source component.
@Alenar
style(aggregator): uniformize function enter logs
7f36129 
Prefix them with a `>> ` followed by the rust function name (including
the `_`) so they can be search easily.
@Alenar
style(aggregator): uniformize http server logs
2c7e096 
Prefix enter function logs with the http verb followed by the route
path.
@Alenar
style(aggregator): Capitalize first letter of all "functional" logs
a73c924
@Alenar
refactor(aggregator): move huge context data away from logs main msg
9080e21 
And instead add them to a property. This allow to keep the main log
messages leans while still providing detailed errors info.
@Alenar
refactor(aggregator): move errors away from main state machine logs msg
e86fa7e 
And write them to an isolated `error` property.
@Alenar
style(signer): align state machine logs system with aggregator
fd24f7d
@Alenar
style(aggregator): reduce some logs space
2b840e2
@Alenar
refactor(aggregator): general log function for http server route calls
b409dd0 
There is a caveat: payload and query strings can't be logs this way, so
in this cas a local "function enter" log is done.
This ensure that those data are logged at the price of routes producing
two logs for one call.
@Alenar
style: adjustements following PR reviews
b5efb83 
Adjust some logs messages & added properties
@Alenar
chore: upgrade crate versions
20156b9 
* mithril-aggregator from `0.5.82` to `0.5.83`
* mithril-signer from `0.2.199` to `0.2.200`
@Alenar Alenar force-pushed the djo/1981/rework-aggregator-log-msgs branch from 80a4792 to 20156b9 Compare October 15, 2024 09:56
@Alenar Alenar merged commit 0d4d6bc into main Oct 15, 2024
45 of 46 checks passed
@Alenar Alenar deleted the djo/1981/rework-aggregator-log-msgs branch October 15, 2024 11:43
@Alenar Alenar mentioned this pull request Oct 15, 2024
Merged
9 tasks
@jpraynaud jpraynaud mentioned this pull request Oct 24, 2024
Closed
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sfauvel sfauvel sfauvel approved these changes

@jpraynaud jpraynaud jpraynaud approved these changes

@dlachaume dlachaume dlachaume approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Alenar @sfauvel @jpraynaud @dlachaume