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

cozy-logger: should we refactor cozy-logger? #1399

Open
Ldoppea opened this issue Dec 23, 2021 · 1 comment
Open

cozy-logger: should we refactor cozy-logger? #1399

Ldoppea opened this issue Dec 23, 2021 · 1 comment

Comments

@Ldoppea
Copy link
Member

Ldoppea commented Dec 23, 2021

Current cozy-logger implementation may be troublesome for future maintainers. Here are some questions/remarks I had in my head when reading the code:

  • filterOut() method's name is suggesting that it's filtering something, but it just returns a boolean. Then the caller decides to filter it or not regarding the boolean.
  • filterOut() implementation is suggesting that all filters should return a boolean, but filterSecrets() can only return void or throw an exception.
    • I'm not sure it is a good thing to throw an exception from a logger (imho loggers should never impact an app's behavior). Are we sure we want this?
  • What is the secret log level for? It is not documented, and filters prevent it to be called. Is it used somewhere?
  • filters logic is inverted compared to filterOut
    • if filterOut returns true : object should be filtered
    • If a filter (filterLevel) returns true : object should NOT be filtered
  • filterLevel() always return false if the given type does not exist in the levels array. This is intended but I think we should make this behavior more visible (even if this creates redundant code)
  • setNoRetry() seems not to be used (except from unit tests). Is it dead code?
  • To few unit tests are written for this regarding the number of possible combinations

What do you think about this?

I would suggest to refactor this code, but as it is used in a lot of places and as it is stable code (not edited during last 3 years) this choice may be discutable.

@trollepierre
Copy link
Contributor

Moreover I think the log method has 4 required parameters, but we notice 2 to 3 parameters filled for most of its usage. I suggest to make explicit the required parameters, document their use with JSDoc, and add one example in the README

And I don't see the minilog website working but it got mention in our documentation: https://docs.cozy.io/en/cozy-client/logging/ .

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

No branches or pull requests

2 participants