feat: add wildcard support for logger configuration#17627
feat: add wildcard support for logger configuration#17627ankitsharma101 wants to merge 6 commits into
Conversation
dabcea8 to
f047a40
Compare
sdirix
left a comment
There was a problem hiding this comment.
Thanks for this first implementation. Please see my comments.
|
Thanks for the thorough review, Stefan! I've fully addressed your feedback across the board: Memory Leak: Added a cache.clear() call inside slurpLogConfigFile so the cache cleanly wipes whenever a user modifies the log configuration file at runtime. Performance Optimization: Refactored logLevelFor to execute a fast O(1) perfect match lookup upfront. If an exact match isn't present, it iterates through the keys backward and short-circuits on the first wildcard match to respect the "last one wins" specificity rule efficiently. Test Refactor: Cleaned up the testing architecture. The wildcard tests no longer mutate private state using as unknown as Record<string, unknown> and instead leverage a helper that tests everything cleanly via the public CLI API. Sensible Defaults: Updated the browser example log-config.json with a lightweight, realistic use case that won't obscure verbose logging during core active AI development. Documentation: Documented the newly introduced wildcard feature, precedence tiers, and exact match priority within packages/core/README.md. I also ran a full clean installation and environment purge locally—the entire core test suite is passing perfectly. Ready for another pass whenever you are! |
sdirix
left a comment
There was a problem hiding this comment.
Documentation: Documented the newly introduced wildcard feature, precedence tiers, and exact match priority within packages/core/README.md.
The documentation change is not part of the current PR. Please add it.
56f6ef5 to
577e822
Compare
|
Everything is updated now. I reverted the unintended changes, got the wildcard docs into the README_TEMPLATE.md and passed all CI/CD tests and fixed a bug in regex matcher. Is everything good now? Will be working on the other PR in the meantime |
sdirix
left a comment
There was a problem hiding this comment.
Code looks good! I just have comments regarding the script, test and documentation
| "coverage:report": "nyc report --reporter=html", | ||
| "rebuild": "theia rebuild:browser --cacheRoot ../..", | ||
| "start": "theia start --plugins=local-dir:../../plugins --ovsx-router-config=../ovsx-router-config.json", | ||
| "start": "theia start --plugins=local-dir:../../plugins --ovsx-router-config=../ovsx-router-config.json --log-config=log-config.json", |
There was a problem hiding this comment.
I realized that this conflicts with the --log-level=debug of the start:debug script. I think we should introduce a new start:plugins script (with the current content of start) and then in start we do npm run -s start:plugins -- --log-config=log-config.json and in start:debug we also use start:plugins
| await cli.setArguments(args); | ||
| } | ||
|
|
||
| it('should respect exact matches if they are evaluated last (or only)', async () => { |
There was a problem hiding this comment.
Test name is misleading: this case has only an exact entry, so it never exercises the "evaluated last"
| "terminal": "debug", | ||
| "task": "error" | ||
| "task": "error", |
There was a problem hiding this comment.
Task and terminal will no longer work, so I would use wildcards for them too
| "ai-core:DefaultPromptFragmentCustomizationService": "debug", | ||
| "ai-core*": "error", |
There was a problem hiding this comment.
I would change their order just to establish the generic to specialized recommendation
| "ai-core:DefaultPromptFragmentCustomizationService": "debug", | |
| "ai-core*": "error", | |
| "ai-core*": "error", | |
| "ai-core:DefaultPromptFragmentCustomizationService": "debug", |
| "terminal": "debug", | ||
| "task": "error" | ||
| "task": "error", | ||
| "ai-core:DefaultPromptFragmentCustomizationService": "debug", | ||
| "ai-core*": "error", | ||
| "*Token*": "warn" |
…wildcard support and precedence rules to loging config and
577e822 to
0a25f90
Compare
|
I've addresssed the script conflicts and renamed the tests. I have also updated the wildcard ocumentation as per the suggestions. Ready for the re-review. |
sdirix
left a comment
There was a problem hiding this comment.
Looks good. However I saw that we already have a regex escape util, so we should reuse that.
| protected getWildcardRegex(pattern: string): RegExp { | ||
| let regex = this.wildcardRegexCache.get(pattern); | ||
| if (!regex) { | ||
| const escapedPattern = pattern.replace(/[.+?^${}()|[\]\\]/g, '\\$&'); |
There was a problem hiding this comment.
Hi @ankitsharma101, I noticed that we already have a util for this:
theia/packages/core/src/common/strings.ts
Line 113 in d6f4563
*, so you can split on the wildcard first and reuse it: pattern.split('*').map(escapeRegExpCharacters).join('.*').
There was a problem hiding this comment.
Thanks for pointing that out, Stefan! I've removed the custom regex and am reusing escapeRegExpCharacters instead. Let me know if everything looks good to merge now!
What it does
Fixes #17466
This PR introduces wildcard and regex support for logger configuration, allowing developers to configure multiple loggers using a single rule (e.g.,
"ai-core*": "error","*Token*": "warn") without needing to explicitly list every sub-service.Implementation Details:
wildcardRegexCachetoLogLevelCliContributionto ensure regex compilation only happens once per pattern for optimal performance.logLevelForto iterate through configured levels, respecting the "last one wins" rule for wildcard specificity.getWildcardRegexto safely escape standard regex characters in the user's config before injecting the.*wildcard matcher.setArgumentstests to ensure strict type compliance and reliable CLI fallbacks.How to test
1. Automated Tests
Run the core test suite to verify the new wildcard matching logic and the patched legacy tests:
yarn --cwd packages/core test2. End-to-End Test
A sample
log-config.jsonhas been added to the browser example.cd examples/browseryarn buildyarn startSuccessfully read new log config from .../log-config.jsonand notice that the plugin loggers perfectly respect the wildcard rules defined in the file.Follow-ups
None currently identified. The regex cache ensures this scales well even if developers provide massive configuration files.
Breaking changes
Attribution
Review checklist
nlsservice (for details, please see the Internationalization/Localization section in the Coding Guidelines)Reminder for reviewers