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.

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

Implement logging framework #88

Merged

Conversation

julianheckmann
Copy link
Contributor

@julianheckmann julianheckmann commented Jan 2, 2024

This PR provides the following:

  • Logging for the app as well as the plugins
  • The plugins receive a plugin context to be used for scoped services which in this case is a logging service
  • To enable logging in the web console (good for debugging) as well as in the log files the renderer sends an IPC event to the main process which then logs it to log files within the poestack folder
  • The logs are grouped by date

This PR resolves: #20

name: string
};

export type PluginContext = {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://github.com/PoeStack/poestack-sage/blob/main/src/echo-common/src/echo-context.ts#L14 this should be used instead, each plugin already has its own instance created in https://github.com/PoeStack/poestack-sage/blob/main/src/echo-app/src/renderer/src/echo-context-factory.ts which they use for these types of services. The contextSource should be used as the name I don't think this is set currently but should be easy enough to update I think right now it just always passes in "plugin".

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea I saw this too. But I thought pushing a context instead of pulling is better. That way the plugins are isolated and may not use the context of other plugins, this is important so the logging service scoping is actually working how we want :). Additionally you don't have to call the echo context service manually with a string.

But if both of you @zach-herridge & @C3ntraX think the echo context factory is a better approach, I can also go with this way. If we decide that the push option is the better one I can integrate it into the plugins.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The issue with pushing is that it's pretty easy for things to happen in the plugin before the entry is called so if they can't access their context before the entry method they are more limited. They could access another plugins context but we wouldn't approve a PR doing that without a really good reason.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright, makes sense. I changed it to use the echo context factory.

@@ -0,0 +1,35 @@
import pino, { Logger, LoggerOptions } from 'pino'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@C3ntraX are you okay with this library?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah saw this. I don't know pino. It seems like a more lightweight version.
@julianheckmann Why did you choosed this lib and have you experience with other libs like winston?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@C3ntraX pino is not only more lightweight but also much more faster. It was created by one of node.js core contributors. Here you can see some benchmarks. It is growing a lot in popularity, currently at 2m downloads. And since it covers all of the use-cases we currently need why not go with it :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems fine to me. We don't need colorization with morgan or different transport channels with or without docker etc. We need just production or not. It should be good enough and the benchmarks are good.

@zach-herridge zach-herridge merged commit 490f180 into PoeStack:main Jan 3, 2024
1 check failed
@julianheckmann julianheckmann deleted the feature/implement-logging branch January 3, 2024 21:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Setup logging framework.
3 participants