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

PoC for log library surface reduction (2991) #2992

Open
wants to merge 8 commits into
base: master
from

Conversation

Projects
None yet
9 participants
@DaanHoogland
Copy link
Contributor

commented Nov 1, 2018

Description

part of #2991, this is the initial step to unifying the log libraries and put the surface in a single location. Note that even for this one functionality about half the files in the entire project will be touched in the end. This one and the StringUtils one are probably the biggest. This and the gson one will be the more complicated than StringUtils. The DAO framework will have even more impact but the may be out of scope for #2991

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)

Screenshots (if appropriate):

How Has This Been Tested?

@DaanHoogland

This comment has been minimized.

Copy link
Contributor Author

commented Nov 1, 2018

@blueorangutan package

@blueorangutan

This comment has been minimized.

Copy link

commented Nov 1, 2018

@DaanHoogland a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

logger.info(s);
}

public void error(String s) {

This comment has been minimized.

Copy link
@marcaurele

marcaurele Nov 1, 2018

Member

Can you add apublic void error (String msg, Throwable e) and change the public void warn(String s, Exception e) to public void warn(String s, Throwable e)

This comment has been minimized.

Copy link
@DaanHoogland

DaanHoogland Nov 1, 2018

Author Contributor

I will as we progress change the interface to what we really use. I am also still considering to use an actual java-interface. Unfortunately java requires an explicit 'implements', otherwise the proxy mechs would be some easier.

@marcaurele

This comment has been minimized.

Copy link
Member

commented Nov 1, 2018

Good change to move all the logs under a single class to ease any future changes or choice of library.

@blueorangutan

This comment has been minimized.

Copy link

commented Nov 1, 2018

Packaging result: ✔centos6 ✔centos7 ✔debian. JID-2403

@DaanHoogland

This comment has been minimized.

Copy link
Contributor Author

commented Nov 2, 2018

@blueorangutan

This comment has been minimized.

Copy link

commented Nov 2, 2018

@DaanHoogland a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests

@blueorangutan

This comment has been minimized.

Copy link

commented Nov 2, 2018

Trillian test result (tid-3140)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 19524 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr2992-t3140-kvm-centos7.zip
Intermittent failure detected: /marvin/tests/smoke/test_multipleips_per_nic.py
Intermittent failure detected: /marvin/tests/smoke/test_vpc_redundant.py
Smoke tests completed. 67 look OK, 2 have error(s)
Only failed tests results shown below:

Test Result Time (s) Test File
test_nic_secondaryip_add_remove Error 21.10 test_multipleips_per_nic.py
test_04_rvpc_network_garbage_collector_nics Failure 437.02 test_vpc_redundant.py
@DaanHoogland

This comment has been minimized.

Copy link
Contributor Author

commented Nov 2, 2018

@csquire

This comment has been minimized.

Copy link
Contributor

commented Nov 6, 2018

Why not use SLF4J instead of reinventing your own facade?
As a developer of an internal plugin, I find it frustrating to be forced into using limited one-off implementations. In particular, I am sending managment server logs to ELK in a JSON format. With SLF4J, Logback with the Logstash Logback Encoder, I can use Markers to add event-specific custom fields wherever I want. This is very useful for searching and filtering logs in ELK, not to mention the potential that exists for visualization of log events based on custom JSON fields. I assume a similar thing can be done with SLF4J and log4j2, but I have not researched it much. But, if Cloudstack uses a homegrown facade, it will not have a full logging feature set available.

@DaanHoogland

This comment has been minimized.

Copy link
Contributor Author

commented Nov 6, 2018

@csquire hearsay admittedly but apache logging folk say that log4j2 is better faster neater than slf4j and when we consolidated the util package we can decide to wrap slf4j in it as well, right now slf4j is used at some places in the system, java logging at others and log4j version 1 mostly. I want to consolidate all logging after merging this PoC first and than upgrade to whatever we decide. The idea here is that we have only one place where the version of the 3rd party lib matters.

@csquire

This comment has been minimized.

Copy link
Contributor

commented Nov 6, 2018

Understood. I'd just like plugin development to be taken into consideration and by using at thin wrapper around logging that doesn't implement a full feature set, plugin developers would have to either bypass it to do what they want or accept that they cannot do certain things. I also understand that I'm probably in the minority (as far as Cloudstack goes) of wanting to do things like add event-specific custom fields to log events.

@DaanHoogland

This comment has been minimized.

Copy link
Contributor Author

commented Nov 6, 2018

happy to work with you on what you need, @csquire . We do want to implement a large enough subset so all are served. ATM we have a maintenance problem, removal of which is our first objective. If you add an issue for your requirements I will add it if my puny abilities allow. Do not see the implementation of this PoC as the final stage or full interface.

@csquire

This comment has been minimized.

Copy link
Contributor

commented Nov 6, 2018

Will do, thanks!

@chiradeep

This comment has been minimized.

Copy link
Contributor

commented Nov 9, 2018

+1

@rhtyd

This comment has been minimized.

Copy link
Member

commented Nov 12, 2018

@DaanHoogland I'll evaluate this soon and get back to you, thanks for the PR!

@rhtyd

This comment has been minimized.

Copy link
Member

commented Nov 13, 2018

@blueorangutan package

@blueorangutan

This comment has been minimized.

Copy link

commented Nov 13, 2018

@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

@DaanHoogland

This comment has been minimized.

Copy link
Contributor Author

commented Nov 13, 2018

hm, there where packages for the latest commit already. test had been run as well.

@blueorangutan

This comment has been minimized.

Copy link

commented Nov 13, 2018

Packaging result: ✔centos6 ✔centos7 ✔debian. JID-2436

package org.apache.cloudstack.utils.log;

public class LogFactory {
// for log4j 1.2.x the actual LogFactory is org.apache.log4j.LogManager

This comment has been minimized.

Copy link
@rhtyd

rhtyd Nov 13, 2018

Member

Indent comment, may get checkstyle issue.

This comment has been minimized.

Copy link
@DaanHoogland

DaanHoogland Nov 13, 2018

Author Contributor

will make sure to indent


public class AutoCloseableUtil {
private final static Logger s_logger = Logger.getLogger(AutoCloseableUtil.class);
private final static Logger s_logger = LogFactory.getLogger(AutoCloseableUtil.class);

This comment has been minimized.

Copy link
@rhtyd

rhtyd Nov 13, 2018

Member

I guess we'll need to refactor codebase wide to use LogFactory.

This comment has been minimized.

Copy link
@DaanHoogland

DaanHoogland Nov 13, 2018

Author Contributor

I would regard that out of scope for this PoC, but part of a series of follow-up PRs.

@rhtyd

rhtyd approved these changes Nov 13, 2018

Copy link
Member

left a comment

LGTM, but I would like to see codebase wide refactoring i.e. everywhere LogFactory is used to get a logger. Also discuss if we need to add more change (improve logging etc)?

@rafaelweingartner

This comment has been minimized.

Copy link
Member

commented Jan 29, 2019

I do know you are not asking my help ;)

The part I disagree with is the method used to do the so called "reduction of the surface of change". There are other alternatives, which in my opinion work better. I tried to explain, but I do not think you understood what I meant. Therefore, I tried to create an example for you, but I have also not been able to finish it.

@GabrielBrascher

This comment has been minimized.

Copy link
Member

commented Jan 29, 2019

@DaanHoogland thanks for the initiative!

There are still open concerns to address and to be fair, now that I had some time to review it, I am sharing the same thoughts of Rafael.

I will be happy to assist in reviewing deep further and discussing different approaches; however, considering our tight schedule and the lack of reviewers, I am afraid that this one will have to wait for the freeze.

@DaanHoogland

This comment has been minimized.

Copy link
Contributor Author

commented Jan 29, 2019

ok, then let me urge to give this priority. We are using antiquated libraries, as in out of service -, out of support libraries and moving forward will not get easier over time.

If you method is the scripting search and replace of all methods used, I am afraid this is just a one of solution and not an architectural pattern for future stability. Please enlighten me because indeed I do not understand the alternative to be viable.

@GabrielBrascher

This comment has been minimized.

Copy link
Member

commented Jan 29, 2019

I totally agree with you @DaanHoogland, this is an issue to give priority and as soon as we have 4.12.0.0 released this should be one of the priorities among all the CloudStack refactoring efforts.
Unfortunately, I think that for this release it is still a bit raw.

I would like to add that our next LTS should be delivered this year to cover the 4.11 end of life (1 July 2019) [1]. I was absent on this discussions so far but will address some effort as well to help on getting rid of all the antiquated libraries on time to merge into the next LTS.

[1] https://cwiki.apache.org/confluence/display/CLOUDSTACK/LTS

@rafaelweingartner

This comment has been minimized.

Copy link
Member

commented Jan 29, 2019

@DaanHoogland on a second thought. Feel free to do what you would like here ;)

I think I gave a -1, but I will not be able to finish what I started to show you. Therefore, I hereby remove it.

@GabrielBrascher

This comment has been minimized.

Copy link
Member

commented Jan 29, 2019

@blueorangutan package

@blueorangutan

This comment has been minimized.

Copy link

commented Jan 29, 2019

@GabrielBrascher a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

@blueorangutan

This comment has been minimized.

Copy link

commented Jan 29, 2019

Packaging result: ✔centos6 ✔centos7 ✔debian. JID-2577

@GabrielBrascher

This comment has been minimized.

Copy link
Member

commented Jan 29, 2019

@blueorangutan

This comment has been minimized.

Copy link

commented Jan 29, 2019

@GabrielBrascher a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests

@DaanHoogland

This comment has been minimized.

Copy link
Contributor Author

commented Jan 29, 2019

@rafaelweingartner I saw you IM comment and will look into what implementing logging like in spring would mean for our logging. more later

@blueorangutan

This comment has been minimized.

Copy link

commented Jan 30, 2019

Trillian test result (tid-3357)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 29187 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr2992-t3357-kvm-centos7.zip
Intermittent failure detected: /marvin/tests/smoke/test_primary_storage.py
Intermittent failure detected: /marvin/tests/smoke/test_snapshots.py
Intermittent failure detected: /marvin/tests/smoke/test_vm_life_cycle.py
Intermittent failure detected: /marvin/tests/smoke/test_host_maintenance.py
Intermittent failure detected: /marvin/tests/smoke/test_hostha_kvm.py
Smoke tests completed. 65 look OK, 5 have error(s)
Only failed tests results shown below:

Test Result Time (s) Test File
test_01_add_primary_storage_disabled_host Error 0.91 test_primary_storage.py
test_01_primary_storage_nfs Error 0.14 test_primary_storage.py
ContextSuite context=TestStorageTags>:setup Error 0.25 test_primary_storage.py
test_02_list_snapshots_with_removed_data_store Error 1.17 test_snapshots.py
test_11_migrate_vm Error 19.00 test_vm_life_cycle.py
test_14_secure_to_secure_vm_migration Error 62.84 test_vm_life_cycle.py
test_01_cancel_host_maintenace_with_no_migration_jobs Failure 0.12 test_host_maintenance.py
test_02_cancel_host_maintenace_with_migration_jobs Error 2.34 test_host_maintenance.py
test_hostha_kvm_host_degraded Failure 664.27 test_hostha_kvm.py
test_hostha_kvm_host_fencing Failure 620.28 test_hostha_kvm.py
test_hostha_kvm_host_recovering Failure 623.50 test_hostha_kvm.py
@DaanHoogland

This comment has been minimized.

Copy link
Contributor Author

commented Feb 7, 2019

used a addepted version of @rafaelweingartner's script above :

#!/bin/bash
CLOUDSTACK_FOLDER="."

ALL_CLOUDSTACK_CLASSES=$(find $CLOUDSTACK_FOLDER -name "*.java")

for class in $ALL_CLOUDSTACK_CLASSES
do
  count=$((count+1))
  echo "Processing ($count) class: $class"

  echo "Replacing org.apache.log4j.Level with org.apache.cloudstack.utils.log.Level"
  sed -i .loch 's/org.apache.log4j.Level/org.apache.cloudstack.utils.log.Level/g' "$class"

  echo "Replacing org.apache.log4j.Logger with org.apache.cloudstack.utils.log.Logger"
  sed -i .loch 's/org.apache.log4j.Logger/org.apache.cloudstack.utils.log.Logger/g' "$class"

  echo "Replacing java.util.logging.Logger with org.apache.cloudstack.utils.log.Logger"
  sed -i .loch 's/java.util.logging.Logger/org.apache.cloudstack.utils.log.Logger/g' "$class"

  echo "Adding import to org.apache.cloudstack.utils.log.LogFactory"
  sed -i .loch '/import org.apache.cloudstack.utils.log.Logger/a \
import org.apache.cloudstack.utils.log.LogFactory;
' "$class"

  echo "Replacing Logger.getLogger with LogFactory.getLogger"
  sed -i .loch 's/Logger.getLogger/LogFactory.getLogger/g' "$class"

  echo "Replacing org.slf4j.Logger with org.apache.cloudstack.utils.log.Logger"
  sed -i .loch 's/org.slf4j.Logger/org.apache.cloudstack.utils.log.Logger/g' "$class"

  echo "Replacing org.slf4j.LoggerFactory with org.apache.cloudstack.utils.log.LogFactory"
  sed -i .loch 's/org.slf4j.LoggerFactory/org.apache.cloudstack.utils.log.LogFactory/g' "$class"

  echo "Replacing s_logger with LOG"
  sed -i .loch 's/s_logger/LOG/g' "$class"
done

the above scripts leaves some to be desired like:

  1. I don't have a constructor taking a String, yet. there where only a handful of those and all used a Class.getname, so I converted the calls instead.
  2. on occasion the script led to a double insertion of the factory import. fixed those by hand as well

@DaanHoogland DaanHoogland force-pushed the shapeblue:log-surface-reduction branch from 08a5937 to dbde366 Feb 8, 2019

@DaanHoogland

This comment has been minimized.

Copy link
Contributor Author

commented Feb 8, 2019

@blueorangutan package

@blueorangutan

This comment has been minimized.

Copy link

commented Feb 8, 2019

@DaanHoogland a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

@blueorangutan

This comment has been minimized.

Copy link

commented Feb 8, 2019

Packaging result: ✔centos6 ✔centos7 ✔debian. JID-2601

@DaanHoogland

This comment has been minimized.

Copy link
Contributor Author

commented Feb 8, 2019

@blueorangutan

This comment has been minimized.

Copy link

commented Feb 8, 2019

@DaanHoogland a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests

@DaanHoogland

This comment has been minimized.

Copy link
Contributor Author

commented Feb 11, 2019

framework/managed-context uses slf4j and is not dependend on utils. This needs to be solved by

  1. isolating logging in its own project and make it second to checkstyle
  2. exclude managed context from the standard use of logging
@borisstoyanov
Copy link
Contributor

left a comment

LGTM

@rhtyd

This comment has been minimized.

Copy link
Member

commented Feb 20, 2019

Trillian env failed to deploy twice, I've rekicked the job again. cc @DaanHoogland

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.