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

Should logger log in process.env.NODE_ENV === test? #102

Closed
oliviertassinari opened this issue Jul 24, 2020 · 9 comments · Fixed by #117
Closed

Should logger log in process.env.NODE_ENV === test? #102

oliviertassinari opened this issue Jul 24, 2020 · 9 comments · Fixed by #117
Labels
component: data grid This is the name of the generic UI component, not the React module! priority: important This change can make a difference

Comments

@oliviertassinari
Copy link
Member

Noticed it with oliviertassinari/material-ui@9a3c143.

@dtassone
Copy link
Member

I think it can log from the info level. So it's less verbose and we can understand a bit more where it fails

@oliviertassinari
Copy link
Member Author

oliviertassinari commented Jul 27, 2020

I was looking that the console on the storybook side, the debug and info messages hide the other important warnings and errors.

What about the following plan?

  • We don't log debug nor info by default, nor in people's applications, nor in our tests, nor in the storybook.
  • We allow developers to opt-in for debug and info levels.

@oliviertassinari oliviertassinari added the priority: important This change can make a difference label Jul 27, 2020
@dtassone
Copy link
Member

How about we set the default level to warning?
If a user use the grid and don't see the warning or errors of the component, it's not great.

How do you want to opt-in?

@oliviertassinari
Copy link
Member Author

oliviertassinari commented Jul 27, 2020

How about we set the default level to warning?
If a user use the grid and don't see the warning or errors of the component, it's not great.

Agree! I was under the assumption that we use console.warn and console.error directly for these feedback, bypassing any abstraction.

How do you want to opt-in?

I think that it depends on the use cases. Maybe?

  • If it's for internal development, we could imagine an env variable or a localeStorage variable.
  • If it's for our users, we could image a prop too.

@dtassone
Copy link
Member

Users have the option to pass their own logger, in the case they use a database or something like seq or splunk...
The default logger used by the grid is console. So by default it does console.warn but it can be overwritten to work with a real logger.
They can also set the minimum level they want the grid to log.

If it's for internal development, we could imagine an env variable or a localeStorage variable.

It already does that. If you add DEBUG in your local storage, it overrides the environment conf and start logging.

What are we trying to achieve here?
Should we just change the default log level to warn?

@oliviertassinari
Copy link
Member Author

oliviertassinari commented Jul 27, 2020

What are we trying to achieve here?
Should we just change the default log level to warn?

This would be great :). It would solve the flooding pain point and be enough to close the issue.


Users have the option to pass their own logger, in the case they use a database or something like seq or splunk...

Interesting, would it be used for remote debugging? For this use case, I was expecting that developers would hook into the native console calls and record all the events. To make sure they don't miss any from other parts of the UI.

@dtassone
Copy link
Member

Well I have not come through the practice of overriding the console to get the component logs. Usually library or component logging get dropped and forgotten as we expect libraries to work...
I've used https://github.com/trentm/node-bunyan in the past which was pushing to our seq interface https://datalust.co/

@oliviertassinari
Copy link
Member Author

oliviertassinari commented Jul 27, 2020

I never had this case in the browser, so I can't tell. I also didn't benchmark what alternative UI libraries have for logging. Let's see and explore how that custom logger will turn out :).

The closest I was in the past, was with Sentry, they wrap EVERYTHING, we would use the console logs recorded to debug exceptions. On node.js side, I have tried node bunyan but their decoder wasn't forwarding the right system signal for our docker instance cleanup to work correctly. We went with a custom implementation and an adapter for ClouldWatch on AWS.

What problem were you solving with Seq? I mean, how was it used to improve the final application?

@dtassone
Copy link
Member

It was used in support for example, to track user actions on the app.
It was also used on the backend so we could track the full user journey.

@oliviertassinari oliviertassinari added the component: data grid This is the name of the generic UI component, not the React module! label Jul 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: data grid This is the name of the generic UI component, not the React module! priority: important This change can make a difference
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants