feat(registry): add structured logs support #531
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
stas-nc
requested review from
blackrabbit99,
b1ff,
yehor-manzhula and
ValikDev
stas-nc
force-pushed
the
feature/plugins
branch
3 times, most recently
from
00cb74e to
56084f2
Compare
b1ff
requested changes
Sep 1, 2023
registry/server/index.ts
Outdated
| @@ -1,16 +1,39 @@ | |||
| import path from 'path'; | |||
|
|
|||
| ['log', 'warn', 'error'].forEach(function (method) { | |||
There was a problem hiding this comment.
this code looks a little bit not transparent to me. Could you please leave a comment why is it written in this way and explaining what does it do
registry/server/index.ts
Outdated
|
|
||
| (async () => { | ||
| try { | ||
| loadPlugins(); |
There was a problem hiding this comment.
I see that loadPlugins() is called 2 times here and inside app(). Is it done intentionally?
|
|
||
| const loggerProxyHandler: ProxyHandler<Logger> = { | ||
| get(target, prop) { | ||
| const requestIdKey = getPluginManagerInstance().getReportingPlugin().requestIdLogLabel ?? 'operationId'; |
There was a problem hiding this comment.
it seems that this one is needed only inside if body
registry/server/util/logger.ts
Outdated
| get(target, prop) { | ||
| const requestIdKey = getPluginManagerInstance().getReportingPlugin().requestIdLogLabel ?? 'operationId'; | ||
| const origMethod = hasKey(target, prop) && target[prop]; | ||
| if (typeof origMethod === 'function') { |
There was a problem hiding this comment.
could you please invert condition to reduce nesting. And probably in order not to break to much, the else case should be handled as well
|
|
||
| export function loadPlugins() { | ||
| if (!pluginManagerInstance) { | ||
| pluginManagerInstance = new PluginManager(...manifest.manifest.plugins); |
There was a problem hiding this comment.
is there any potential issue to initialize it directly in module?
i.e. const pluginManagerInstance = new PluginManager(...)
There was a problem hiding this comment.
it will be triggered in the import ESM module phase, so it makes it harder to control flow.
stas-nc
force-pushed
the
feature/plugins
branch
4 times, most recently
from
236ced2 to
e1cf6bb
Compare
Add ilc-plugins-sdk partial support
stas-nc
force-pushed
the
feature/plugins
branch
from
e1cf6bb to
1ccba2c
Compare
Coverage ReportIlc/serverCommit SHA:f267a99f64ea16f641031d071d280bbd7f53b624 Test coverage results 🧪File details
Ilc/clientCommit SHA:f267a99f64ea16f641031d071d280bbd7f53b624 Test coverage results 🧪File details
RegistryCommit SHA:f267a99f64ea16f641031d071d280bbd7f53b624 Test coverage results 🧪File details
|
b1ff
approved these changes
Sep 6, 2023
# for free
to join this conversation on GitHub.
Already have an account?
# to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.