Skip to content

conditional logging for prio - #6973

Closed
DaanHoogland wants to merge 1 commit into
apache:mainfrom
shapeblue:logPrio
Closed

conditional logging for prio#6973
DaanHoogland wants to merge 1 commit into
apache:mainfrom
shapeblue:logPrio

Conversation

@DaanHoogland

@DaanHoogland DaanHoogland commented Dec 12, 2022

Copy link
Copy Markdown
Contributor

Description

This PR intents to guard against string evaluation in parameters to low level logging without sacrificing readability.

Types of changes

  • Breaking change (fix or feature that would cause existing functionality to change)
  • New feature (non-breaking change which adds functionality)
  • Bug fix (non-breaking change which fixes an issue)
  • Enhancement (improves an existing feature and functionality)
  • Cleanup (Code refactoring and cleanup, that may add test cases)

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

  • Major
  • Minor

Bug Severity

  • BLOCKER
  • Critical
  • Major
  • Minor
  • Trivial

Screenshots (if appropriate):

How Has This Been Tested?

@DaanHoogland

Copy link
Copy Markdown
Contributor Author

@blueorangutan package

@blueorangutan

Copy link
Copy Markdown

@DaanHoogland a Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

@DaanHoogland DaanHoogland mentioned this pull request Dec 12, 2022
12 tasks
@sonarqubecloud

Copy link
Copy Markdown

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

0.0% 0.0% Coverage
0.0% 0.0% Duplication

@blueorangutan

Copy link
Copy Markdown

Packaging result: ✖️ el7 ✔️ el8 ✖️ debian ✖️ suse15. SL-JID 4858

@GutoVeronezi

Copy link
Copy Markdown
Contributor

@DaanHoogland

I understand your point in avoiding building a String for low-level logging and not using it; however, I think this proposal is not the best option for handling the situation. As you pointed out in the comment #6925 (comment) and from conversations we had along the time, Log4j2 would solve our issues of string evaluation when it is not necessary. Unfortunately, the community could not proceed with the upgrade from Log4j to Log4j2, as it would demand a huge work effort, and also due to the lack of time and having other priorities; however, now we managed to put efforts into this huge change. We separated the whole project into two phases:

  1. Upgrade from Log4j to Log4j2;
  2. Normalize the use of the Log4j2 syntax (as Log4j2 has back compatibility with Log4j, we could separate the scopes);

We are already working on the first phase; however, as it is a huge change, it is going to take some time until releasing the first draft. As soon as we get the first draft done, we are going to open a well-detailed PR explaining the changes and the effects they will have on ACS.

@DaanHoogland

Copy link
Copy Markdown
Contributor Author

@GutoVeronezi I appreciate the work.
Does this mean you do not approve of this change as a short term, intermediate step?

@GutoVeronezi

Copy link
Copy Markdown
Contributor

@DaanHoogland, IMHO, I do not think it worth the effort of adding an intermediate step; however, the changes are not mutually exclusive. Therefore, it is not a -1 at all, it is a -0.

@github-actions

Copy link
Copy Markdown

This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch.

@DaanHoogland

Copy link
Copy Markdown
Contributor Author

clearly voted away

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants