chore: facilitate trace certificates filtering #142
Conversation
WalkthroughThis pull request refines the logging mechanism for certificate-related actions and updates the monitoring documentation. In the ledger module, a new constant Changes
Suggested reviewers
Poem
Tip ⚡🧪 Multi-step agentic review comment chat (experimental)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Signed-off-by: jeluard <[email protected]>
Signed-off-by: jeluard <[email protected]>
jeluard
force-pushed
the
jeluard/better-certificate-traces
branch
from
7260416 to
91cc773
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (2)
monitoring/README.md (2)
225-239: 🛠️ Refactor suggestionFix trace summary section headings
The headings for two trace sections appear to be incorrect. They should match the actual trace names being used in the code.
- <details><summary>trace: `stake.registration`</summary> + <details><summary>trace: `pool.registration`</summary> ... - <details><summary>trace: `stake.retirement`</summary> + <details><summary>trace: `pool.retirement`</summary>These sections are describing pool operations based on the field descriptions, but the headings incorrectly label them as stake operations.
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
230-230: Tables should be surrounded by blank lines
null(MD058, blanks-around-tables)
238-238: Tables should be surrounded by blank lines
null(MD058, blanks-around-tables)
241-265: 🛠️ Refactor suggestionUpdate remaining detail section headings
The remaining detail section headings still contain the "certificate." prefix which should be removed to maintain consistency with the updated trace names.
- <details><summary>trace: `certificate.drep.registration`</summary> + <details><summary>trace: `drep.registration`</summary> ... - <details><summary>trace: `certificate.drep.retirement`</summary> + <details><summary>trace: `drep.retirement`</summary> ... - <details><summary>trace: `certificate.vote.delegation`</summary> + <details><summary>trace: `vote.delegation`</summary>🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
246-246: Tables should be surrounded by blank lines
null(MD058, blanks-around-tables)
255-255: Tables should be surrounded by blank lines
null(MD058, blanks-around-tables)
264-264: Tables should be surrounded by blank lines
null(MD058, blanks-around-tables)
🧹 Nitpick comments (1)
monitoring/README.md (1)
18-18: Fix grammatical error in environment variables descriptionThere's a small grammatical number issue in the tip text.
- > Both environment variable are optional. + > Both environment variables are optional.🧰 Tools
🪛 LanguageTool
[uncategorized] ~18-~18: The grammatical number of this noun doesn’t look right. Consider replacing it.
Context: ...to stdout; > [!TIP] > Both environment variable are optional. > > - When omitted, `AMAR...(AI_EN_LECTOR_REPLACEMENT_NOUN_NUMBER)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
crates/amaru-ledger/src/state/transaction.rs(3 hunks)monitoring/README.md(3 hunks)
🧰 Additional context used
🪛 LanguageTool
monitoring/README.md
[uncategorized] ~14-~14: Loose punctuation mark.
Context: ... environment variables: - AMARU_TRACE: for any event emitted by the OpenTeleme...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~18-~18: The grammatical number of this noun doesn’t look right. Consider replacing it.
Context: ...to stdout; > [!TIP] > Both environment variable are optional. > > - When omitted, `AMAR...
(AI_EN_LECTOR_REPLACEMENT_NOUN_NUMBER)
🪛 markdownlint-cli2 (0.17.2)
monitoring/README.md
207-207: Tables should be surrounded by blank lines
null
(MD058, blanks-around-tables)
215-215: Tables should be surrounded by blank lines
null
(MD058, blanks-around-tables)
222-222: Tables should be surrounded by blank lines
null
(MD058, blanks-around-tables)
230-230: Tables should be surrounded by blank lines
null
(MD058, blanks-around-tables)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Snapshots (preprod, 10.1.4)
- GitHub Check: Build on windows-latest with target x86_64-pc-windows-msvc
🔇 Additional comments (6)
crates/amaru-ledger/src/state/transaction.rs (2)
33-33: Nice addition of dedicated event target for certificates!The new
CERTIFICATES_EVENT_TARGETconstant provides a more focused way to trace certificate operations. This is a cracker of an improvement for devs who need to filter logs specifically for certificate handling.
241-241: LGTM! Consistent update of trace targetsYou've done a brilliant job updating all the trace statements to use the new certificate-specific target. The simplified log messages (removing the redundant "certificate." prefix) make the logs cleaner and easier to scan.
Also applies to: 245-245, 249-249, 255-255, 259-259, 263-263, 293-293, 299-299, 303-303, 307-307
monitoring/README.md (4)
14-15: Environment variable names and descriptions updated correctlyGood job renaming
AMARU_DEV_LOGtoAMARU_TRACEand clarifying the purpose of each variable. This makes the distinction between OpenTelemetry layer events and stdout events crystal clear.🧰 Tools
🪛 LanguageTool
[uncategorized] ~14-~14: Loose punctuation mark.
Context: ... environment variables: -AMARU_TRACE: for any event emitted by the OpenTeleme...(UNLIKELY_OPENING_PUNCTUATION)
20-21: Default log levels updated appropriatelyThe updated default log levels provide better granularity - setting
AMARU_LOGto "debug" level andAMARU_TRACEto "trace" level makes perfect sense for the different use cases.
36-37: Great example addition!Adding a practical example of how to filter events by target and severity is spot on. This will be a lifesaver for developers trying to set up their logging configurations.
183-201: Comprehensive documentation of the new trace targetThe addition of the new
amaru::ledger::state::transaction::certificatestarget section is well-structured and aligns perfectly with the code changes. The simplified trace names (without the "certificate." prefix) match the updated logging statements in the code.
Introduce a new trace target
amaru::ledger::state::transaction::certificatesso that certificate handling can be traced easily.e.g.
AMARU_LOG="amaru::ledger::state::transaction::certificate=trace"Summary by CodeRabbit
Refactor
Documentation