neoave / mrack

Multicloud use-case based multihost async provisioner for CIs and testing during development
Apache License 2.0
11 stars 14 forks source link

Improve mrack logging #45

Closed Tiboris closed 3 years ago

Tiboris commented 3 years ago

refactor: Add dsp_name to logging messages to display context

Add dsp_name to logging messages to display context
for prioviders and transformers.
Added some debug messages.

Aims to resolve https://github.com/neoave/mrack/issues/23

Signed-off-by: Tibor Dudlák tdudlak@redhat.com

Tiboris commented 3 years ago
mrack --debug

yes you are correct when looking at this it should raise exception https://github.com/neoave/mrack/blob/master/src/mrack/run.py#L194 and it raises it with the known_error when file is not found seems to be the: <class 'mrack.errors.ConfigError'> where it breaks, seems like exception_handler can not use logger with my changes.

Tiboris commented 3 years ago

@netoarmando I hope it is fixed, reason for that can be seen in file __init__.py https://github.com/neoave/mrack/pull/45/files#diff-3b06fe18a2bbf59c91431116c264ffac811c2e81bf485eed06ca51536c7e896cR15 The decorator does not pass the right function name for the logger.

pvoborni commented 3 years ago

Imagine that you are a person who don't know the implementation of mrack. What is your feeling as the user when you are using mrack with this logging?

Tiboris commented 3 years ago

Imagine that you are a person who don't know the implementation of mrack. What is your feeling as the user when you are using mrack with this logging?

Could you please collaborate more on what do you have in mind? I am afraid that I do not understand what you are trying to point out. :(

pvoborni commented 3 years ago

Imagine that you are a person who don't know the implementation of mrack. What is your feeling as the user when you are using mrack with this logging?

Could you please collaborate more on what do you have in mind? I am afraid that I do not understand what you are trying to point out. :(

It's a question to you and Armando :) Don't want to point out anything yet. Sort of a nudge to apply a perspective different from ours (developers).

netoarmando commented 3 years ago

Imagine that you are a person who don't know the implementation of mrack. What is your feeling as the user when you are using mrack with this logging?

For me, the ideal scenario would be to use python standard logging call, BUT with the option to use our internal logging for context.. However, I do think that we can refer to the Zen of Python, which might mean to return to plain loggers with explicit strings (e.g.: f"{provider}: message" because it is simpler and explicit.

pvoborni commented 3 years ago

I actually never red Zen of Python, thanks for the reference. https://www.python.org/dev/peps/pep-0020/

In the spirit of it I'd add: "Experience for end users is more important than purity of implementation."

I'd say prefixing is desired only in transformers and providers and in debug level. In normal operations, in the rest of the app is not desired - almost no CLI apps do it.

Going with simple logger could be the way. E.g. having a logging wrapper(s) with the prefixing in transformer and provider base class and using them in the logging calls in transformer/provider implementations.

Tiboris commented 3 years ago

From my point of view it is totally all-right to complicate logger little bit so it fulfil our needs and we use the same everywhere and as implementation looks like now, it is not really that complicated. I believe it is okay that project has its own logger.