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

[DPE-2087] Improve logging handling #15

Merged
merged 2 commits into from
Jun 16, 2023

Conversation

deusebio
Copy link
Contributor

No description provided.

@deusebio deusebio marked this pull request as draft June 12, 2023 23:11
@juditnovak
Copy link
Contributor

I'm wondering about a different, perhaps more conventional approach.

You proposed way goes something like this:

  • utility function for logger initialization and retrieval
  • in each script we manually need to initialize the logger, and get the specific one we'd like to use for the script

Instead, why not:

  • initialize the logging for the spark8t app at "startup" (whatever that means: first script execution/API call/etc.) --
  • get the logger the traditional way:
# xyz.py module
<imports> 

logger = logging.getLogger(__name__)

I think this could be a more consistent and "bullet-proof" approach.

@deusebio deusebio marked this pull request as ready for review June 16, 2023 14:00
@juditnovak
Copy link
Contributor

I strongly disagree to this solution, as

  1. It goes against generic Python logging best practices together with our "internal standards" withing the Company. Typically, the concept of https://github.com/deusebio/spark-k8s-toolkit-py/blob/dpe-2087-logging-handling/spark8t/cli/params.py#L140 goes against the conventional solution of used across Canonical charms as well (*), such as
<imports>

logger = logging.getLogger(__name__)
  1. I believe that logging for an application is to be initialized in a single, central, automated, "impossible-to-bypass" stage, for all interfaces/endpoints. Manually initializing logging in every single endpoint doesn't only allow for inconsistencies, but both for bugs and non-standard solutions across the same application.

Due to reason above, I'm not comfortable to give my approval here.

However, of course I understand the pressure to have the functionality available.

I recommend to go ahead my approval here, and I've opened https://warthogs.atlassian.net/browse/DPE-2138 where I'm volunteering to address the issues described above, as my next task to do.

(*) References:

@deusebio deusebio merged commit 37d605d into canonical:main Jun 16, 2023
4 checks passed
@deusebio deusebio deleted the dpe-2087-logging-handling branch June 16, 2023 22:02
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.

None yet

2 participants