feat(editor): Add `CRITICAL` log level as alias for `FATAL` (resolves #212). #218

zzxthehappiest · 2025-03-27T03:22:20Z

Description

This PR is to fix #212 . Add CRITICAL into the log level support and set it has the same severity as FATAL and ERROR. However, the log level filter doesn't work because the clp-ffi-js's decode_range doesn't recoginize CRITICAL as one of its log level, so the log level of CRITICAL log lines is 0 (UNKNOWN). I found it by put a printout in ClpIrDecoder/index.ts:150, but the root cause seems to be in clp-ffi-js.

Checklist

The PR satisfies the contribution guidelines.
This is a breaking change and that has been indicated in the PR title, OR this isn't a
breaking change.
Necessary docs have been updated, OR no docs need to be updated.

Validation performed

In a Python project, install https://github.com/y-scope/clp-loglib-py and add handler CLPFileHandler to a logger. Invoke .critical("some message") as well as fatal (but I found that fatal also appends CRITCAL log, it is essentially using .critical() in the code) and error on the logger to append a CRITICAL level message.
The CRITICAL is bolded and of the same color with FATAL and ERROR

Summary by CodeRabbit

Refactor
- Refined the log entry classification system by unifying the indicators for critical and fatal messages. End-users will experience a consistent visual representation for high-severity logs, helping to better identify important alerts at a glance. This internal improvement does not change overall functionality but enhances user clarity for a better experience.

coderabbitai · 2025-03-27T03:22:30Z

Walkthrough

This change updates the tokenization rules within the setupCustomLogLanguage function in src/components/Editor/MonacoInstance/language.ts. The update replaces the original mapping for the "FATAL" log level with a new mapping that combines "CRITICAL" and "FATAL" into a single token rule (CRITICAL|FATAL). No alterations to exported or public entities were made.

Changes

File	Change Summary
src/components/Editor/…/MonacoInstance/language.ts	Updated tokenization rules in `setupCustomLogLanguage` to combine "CRITICAL" and "FATAL" into `CRITICAL\|FATAL`.

Assessment against linked issues

Objective	Addressed	Explanation
Highlight Python `CRITICAL` log level (#212)	✅

Possibly related PRs

fix(editor): Fully highlight "WARNING" log level (fixes #193). #202: Adjusts tokenization rules for log levels (handling "WARNING" and "WARN"), indicating a related focus on log level tokenization.

Suggested reviewers

junhaoliao

📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between f5b49f1 and 5ccf899.

📒 Files selected for processing (1)

src/components/Editor/MonacoInstance/language.ts (1 hunks)

🚧 Files skipped from review as they are similar to previous changes (1)

src/components/Editor/MonacoInstance/language.ts

🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

Review comments: Directly reply to a review comment made by CodeRabbit. Example:
- I pushed a fix in commit <commit_id>, please review it.
- Generate unit testing code for this file.
- Open a follow-up GitHub issue for this discussion.
Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
- @coderabbitai generate unit testing code for this file.
- @coderabbitai modularize this function.
PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
- @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
- @coderabbitai read src/utils.ts and generate unit testing code.
- @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
- @coderabbitai help me debug CodeRabbit configuration file.

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)

@coderabbitai pause to pause the reviews on a PR.
@coderabbitai resume to resume the paused reviews.
@coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
@coderabbitai full review to do a full review from scratch and review all the files again.
@coderabbitai summary to regenerate the summary of the PR.
@coderabbitai generate docstrings to generate docstrings for this PR.
@coderabbitai resolve resolve all the CodeRabbit review comments.
@coderabbitai plan to trigger planning for file edits and PR creation.
@coderabbitai configuration to show the current CodeRabbit configuration for the repository.
@coderabbitai help to get help.

Other keywords and placeholders

Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (`.coderabbit.yaml`)

You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
Please see the configuration documentation for more information.
If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

Visit our Documentation for detailed information on how to use CodeRabbit.
Join our Discord Community to get help, request features, and share feedback.
Follow us on X/Twitter for updates and announcements.

zzxthehappiest · 2025-03-27T03:24:11Z

Hi @junhaoliao , do I also need to submit an issue/PR in clp-ffi-js repo? Since there seems to be a necessary change to let decoder can set log level correctly.

coderabbitai

Actionable comments posted: 0

🧹 Nitpick comments (1)

src/components/Editor/MonacoInstance/theme.ts (1)

17-17: Consider using a distinct color for CRITICAL in the DARK theme

While the bold styling differentiates CRITICAL from WARN, they currently share the same foreground color ("#ce9178") in the DARK theme. Consider using a unique color for CRITICAL to improve visual distinction, similar to how the LIGHT theme implements different colors between WARN and CRITICAL.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 27020fd and f5b49f1.

📒 Files selected for processing (5)

src/components/Editor/MonacoInstance/language.ts (1 hunks)
src/components/Editor/MonacoInstance/theme.ts (2 hunks)
src/components/Editor/MonacoInstance/typings.ts (1 hunks)
src/components/StatusBar/LogLevelSelect/LogLevelChip.tsx (1 hunks)
src/typings/logs.ts (1 hunks)

🧰 Additional context used

📓 Path-based instructions (1)

`**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}`: - Prefer `false == ` rather than `!`.

**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}: - Prefer false == <expression> rather than !<expression>.

src/typings/logs.ts
src/components/StatusBar/LogLevelSelect/LogLevelChip.tsx
src/components/Editor/MonacoInstance/language.ts
src/components/Editor/MonacoInstance/typings.ts
src/components/Editor/MonacoInstance/theme.ts

🧬 Code Definitions (3)

src/components/StatusBar/LogLevelSelect/LogLevelChip.tsx (1)

src/typings/logs.ts (1)

LOG_LEVEL (57-57)

src/components/Editor/MonacoInstance/language.ts (1)

src/components/Editor/MonacoInstance/typings.ts (1)

TOKEN_NAME (70-70)

src/components/Editor/MonacoInstance/theme.ts (1)

src/components/Editor/MonacoInstance/typings.ts (1)

TOKEN_NAME (70-70)

🔇 Additional comments (5)

src/typings/logs.ts (1)

13-13: Addition of CRITICAL log level looks good

The new log level is appropriately positioned between WARN and ERROR in the severity hierarchy, which aligns with common logging conventions and the PR objective.

src/components/Editor/MonacoInstance/theme.ts (1)

35-35: CRITICAL styling in LIGHT theme looks good

The styling for CRITICAL matches ERROR and FATAL in the LIGHT theme, which correctly represents their equivalent severity levels.

src/components/Editor/MonacoInstance/typings.ts (1)

13-13: Addition of CUSTOM_CRITICAL token name looks good

The new token follows the consistent naming pattern of other log level tokens and is appropriately positioned in the enum.

src/components/Editor/MonacoInstance/language.ts (1)

27-30: CRITICAL tokenization rule implementation looks good

The implementation correctly adds a tokenization rule for the "CRITICAL" string that maps to the new TOKEN_NAME.CUSTOM_CRITICAL. The structure is consistent with other log level tokenization rules.

src/components/StatusBar/LogLevelSelect/LogLevelChip.tsx (1)

21-21: LGTM! Appropriate mapping for CRITICAL log level.

The addition of CRITICAL log level mapped to "danger" color correctly aligns with the PR objective of giving it the same severity as ERROR and FATAL log levels.

junhaoliao

The changes look clean. Nice work!

One thing I noticed is that this introduces an additional log level. A possible alternative could be treating CRITICAL (which is unique to Python’s logging library compared to other common logging frameworks) as an alias for FATAL. You can refer to the existing handling of the WARNING level as an alias for WARN in the TOKEN_NAME.CUSTOM_WARN regex. This way, we can avoid adding extra LOG_LEVEL and token name entries.

Let me know your thoughts, and thanks again for the contribution!

Reference

Python maps FATAL to CRITICAL with an identical severity number:
https://github.com/python/cpython/blob/151d1bfd1bb7ca9a36bb0f2bd6df53d64a1ba2f2/Lib/logging/__init__.py#L99

zzxthehappiest · 2025-03-27T03:36:17Z

The changes look clean. Nice work!

One thing I noticed is that this introduces an additional log level. A possible alternative could be treating CRITICAL (which is unique to Python’s logging library compared to other common logging frameworks) as an alias for FATAL. You can refer to the existing handling of the WARNING level as an alias for WARN in the TOKEN_NAME.CUSTOM_WARN regex. This way, we can avoid adding extra LOG_LEVEL and token name entries.

Let me know your thoughts, and thanks again for the contribution!

Reference

Python maps FATAL to CRITICAL with an identical severity number: https://github.com/python/cpython/blob/151d1bfd1bb7ca9a36bb0f2bd6df53d64a1ba2f2/Lib/logging/__init__.py#L99

Sorry I misunderstood the requirement of this PR before. I will modify it, thanks for the comments :)

zzxthehappiest · 2025-03-27T03:44:33Z

Hi @junhaoliao I think the log level filter does not work correctly for this case. For example, if select "FATAL" the CRITICAL log will be filtered because its log level is 0. I saw a similar one #208 , I guess which is also caused by WARNING is not in clp-ffi-js's decoder, so it is treated as UNKNOWN type.

junhaoliao · 2025-03-27T03:47:29Z

The changes look clean. Nice work!
One thing I noticed is that this introduces an additional log level. A possible alternative could be treating CRITICAL (which is unique to Python’s logging library compared to other common logging frameworks) as an alias for FATAL. You can refer to the existing handling of the WARNING level as an alias for WARN in the TOKEN_NAME.CUSTOM_WARN regex. This way, we can avoid adding extra LOG_LEVEL and token name entries.
Let me know your thoughts, and thanks again for the contribution!

Reference

Python maps FATAL to CRITICAL with an identical severity number: https://github.com/python/cpython/blob/151d1bfd1bb7ca9a36bb0f2bd6df53d64a1ba2f2/Lib/logging/__init__.py#L99

Sorry I misunderstood the requirement of this PR before. I will modify it, thanks for the comments :)

Not a problem! I do see benefits with your proposed solution because it makes sense to show CRITICAL in the Log Level Filter, assuming filtering with CRITICAL is correctly supported.

For now, adding an alias is more of a shortcut, and this shortcut makes sense only because Python's logging library treats FATAL as an alias of CRITICAL. (like we are just doing the other way around - making CRITICAL an alias of FATAL. why didn't the Python logging library author make it the other way around? why would they need the CRITICAL level in the first place? Did they introduce the alias to maintain some compatibility with logj4? we don't really know why they made such decisions 23 years ago... lol)

Before we proceed to support CRITICAL log level filtering, which requires some design changes in the IR format, I believe taking the shortcut in the log-viewer is better for now.

junhaoliao · 2025-03-27T03:51:02Z

Hi @junhaoliao I think the log level filter does not work correctly for this case. For example, if select "FATAL" the CRITICAL log will be filtered because its log level is 0. I saw a similar one #208 , I guess which is also caused by WARNING is not in clp-ffi-js's decoder, so it is treated as UNKNOWN type.

That's right. Internally we have some discussions regarding how that can be addressed, but the potential implementations haven't been shared publicly at y-scope/clp-ffi-js#70 . Let me try to get some help from @LinZhihao-723 to add the descriptions, and we may continue the discussion from there.

zzxthehappiest · 2025-03-27T03:54:33Z

I heard @LinZhihao-723 is one of the busiest guys🥲 If this thinig is not difficult, I could explore and probably add it myself, then just need review. I also want to get myself familar with clp-ffi-js, because this project seems have a deep dependency on that one.

Oops, didn't see you only want to add descriptions.

junhaoliao

For the PR title, how about:

feat(editor): Add `CRITICAL` log level as alias for `FATAL` (resolves #212).

anlowee · 2025-03-27T18:57:29Z

For the PR title, how about:

feat(editor): Add `CRITICAL` log level as alias for `FATAL` (resolves #212).

I changed it

Henry8192

LGTM.

junhaoliao · 2025-03-27T22:03:49Z

I heard @LinZhihao-723 is one of the busiest guys🥲 If this thinig is not difficult, I could explore and probably add it myself, then just need review. I also want to get myself familar with clp-ffi-js, because this project seems have a deep dependency on that one.

Oops, didn't see you only want to add descriptions.

No problem! I have mentioned you in y-scope/clp-ffi-js#70

Let us know if you have any questions there

Add CRITICAL log level with the same severity as FATAL and ERROR

f5b49f1

zzxthehappiest requested a review from a team as a code owner March 27, 2025 03:22

coderabbitai bot reviewed Mar 27, 2025

View reviewed changes

junhaoliao requested changes Mar 27, 2025

View reviewed changes

junhaoliao requested a review from Henry8192 March 27, 2025 03:34

Add CRITICAL log level as an alias of FATAL

5ccf899

junhaoliao approved these changes Mar 27, 2025

View reviewed changes

junhaoliao mentioned this pull request Mar 27, 2025

WARNING logs not included after calling filterLogEvents with LogLevel::WARN y-scope/clp-ffi-js#70

Open

anlowee changed the title ~~fix: Add CRITICAL log level with the same severity as FATAL and ERROR (fixes #212).~~ feat(editor): Add CRITICAL log level as alias for FATAL (resolves #212). Mar 27, 2025

Henry8192 approved these changes Mar 27, 2025

View reviewed changes

junhaoliao merged commit 24ed721 into y-scope:main Mar 27, 2025
5 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(editor): Add `CRITICAL` log level as alias for `FATAL` (resolves #212). #218

feat(editor): Add `CRITICAL` log level as alias for `FATAL` (resolves #212). #218

zzxthehappiest commented Mar 27, 2025 •

edited by coderabbitai bot

Loading

coderabbitai bot commented Mar 27, 2025 •

edited

Loading

Chat

CodeRabbit Commands (Invoked using PR comments)

Other keywords and placeholders

CodeRabbit Configuration File (`.coderabbit.yaml`)

Documentation and Community

zzxthehappiest commented Mar 27, 2025

coderabbitai bot left a comment

junhaoliao left a comment •

edited

Loading

zzxthehappiest commented Mar 27, 2025

Reference

zzxthehappiest commented Mar 27, 2025

junhaoliao commented Mar 27, 2025

Reference

junhaoliao commented Mar 27, 2025

zzxthehappiest commented Mar 27, 2025 •

edited

Loading

junhaoliao left a comment

anlowee commented Mar 27, 2025

Henry8192 left a comment

junhaoliao commented Mar 27, 2025

feat(editor): Add CRITICAL log level as alias for FATAL (resolves #212). #218

feat(editor): Add CRITICAL log level as alias for FATAL (resolves #212). #218

Conversation

zzxthehappiest commented Mar 27, 2025 • edited by coderabbitai bot Loading

Description

Checklist

Validation performed

Summary by CodeRabbit

coderabbitai bot commented Mar 27, 2025 • edited Loading

Walkthrough

Changes

Assessment against linked issues

Possibly related PRs

Suggested reviewers

Chat

CodeRabbit Commands (Invoked using PR comments)

Other keywords and placeholders

CodeRabbit Configuration File (.coderabbit.yaml)

Documentation and Community

zzxthehappiest commented Mar 27, 2025

coderabbitai bot left a comment

Choose a reason for hiding this comment

junhaoliao left a comment • edited Loading

Choose a reason for hiding this comment

Reference

zzxthehappiest commented Mar 27, 2025

Reference

zzxthehappiest commented Mar 27, 2025

junhaoliao commented Mar 27, 2025

Reference

junhaoliao commented Mar 27, 2025

zzxthehappiest commented Mar 27, 2025 • edited Loading

junhaoliao left a comment

Choose a reason for hiding this comment

anlowee commented Mar 27, 2025

Henry8192 left a comment

Choose a reason for hiding this comment

junhaoliao commented Mar 27, 2025

feat(editor): Add `CRITICAL` log level as alias for `FATAL` (resolves #212). #218

feat(editor): Add `CRITICAL` log level as alias for `FATAL` (resolves #212). #218

zzxthehappiest commented Mar 27, 2025 •

edited by coderabbitai bot

Loading

coderabbitai bot commented Mar 27, 2025 •

edited

Loading

CodeRabbit Configuration File (`.coderabbit.yaml`)

junhaoliao left a comment •

edited

Loading

zzxthehappiest commented Mar 27, 2025 •

edited

Loading