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

NEWRELIC-3836 remove obsolete ibm workaround - support Semeru/OpenJ9 versions #993

Merged
merged 1 commit into from
Oct 17, 2022

Conversation

tbradellis
Copy link
Contributor

@tbradellis tbradellis commented Aug 30, 2022

Issue explained in these comments:
We never add the agent-bridge-datastore.jar in the ibmWorkaround path. However, even with that, there is still a bug in the IBM Workaround path where we fail to use the correct branch of logic for versions 8 and above.

         // Check for the IBM workaround
            boolean ibmWorkaround = IBMUtils.getIbmWorkaroundDefault();
            if (System.getProperty("ibm_iv25688_workaround") != null) {
                ibmWorkaround = Boolean.parseBoolean(System.getProperty("ibm_iv25688_workaround"));
            }

            ClassLoader classLoader;
            if (ibmWorkaround) {
                // For the IBM workaround lets just use the System ClassLoader
                //if you come down this path, we never end up
                //performing addBridgeJarToClassPath(inst, AGENT_BRIDGE_DATASTORE_JAR_NAME); hence NoClassDefFound for Record.Sql
                classLoader = ClassLoader.getSystemClassLoader();
            } else {
                ClassLoader agentClassLoaderParent = getPlatformClassLoaderOrNull();

                // Create a new URLClassLoader instance for the agent to use instead of relying on the System ClassLoader
                URL[] codeSource;
                if (isJavaSqlLoadedOnPlatformClassLoader(javaVersion)) {
                    //Semeru and other OpenJ9s for JDK 9+ are not getting to this path, and they should be.
                    //the logic in getIbmWorkaroundDefault() is faulty and causes anything above JDK 7 to not return false
                    URL url = BootstrapLoader.getDatastoreJarURL();
                    codeSource = new URL[] { getAgentJarUrl(), url };
                } else {
                    //If you get here, then the BootstrapLoader will have already
                    // performed addBridgeJarToClassPath(inst, AGENT_BRIDGE_DATASTORE_JAR_NAME); so, no problem (i.e. older JDKs did this).  
                    //IBM JDK 8 versions should have ended up here too, but did not because of the bug in  getIbmWorkaroundDefault() is faulty 
                  
                    codeSource = new URL[] { getAgentJarUrl() };
                }

                classLoader = new JVMAgentClassLoader(codeSource, agentClassLoaderParent);

                redefineJavaBaseModule(inst, classLoader);
                addReadUnnamedModuleToHttpModule(inst, agentClassLoaderParent);
            }
            
            

You can see in the if condition here that we only really check 1.7, so IBM JVMs 8 and above will always be true. 8 is above srNum 4. Also, 9+ uses platform classloader, so this is all just uneccesary. Had this check been correct, this issue probably wouldn't have come up. Still, best to rip this stuff out because it just is pointless complexity in the BootstrapAgent.

    public static boolean getIbmWorkaroundDefault() {
        try {
            if (isIbmJVM()) {
                String jvmVersion = System.getProperty("java.specification.version", "");
                int srNum = getIbmSRNumber();
                //We only set to false if 1.7 and above srNum
                //that means anything greater than jdk 7 will go through the true branch.
                if ("1.7".equals(jvmVersion) && srNum >= 4) {
                    // IBM Ticket says 7.SR3 is fixed, but no jvm can be found to test against
                    return false;
                }
                return true;
            }
            return false;
        } catch (Exception e) {
            return true;
        }
    }

This entire logic is obsolete anyway because the agent no longer supports Java 6 and 7.

Tests:
We need to add OpenJ9 and/or Semeru runtimes to our AITs

@tbradellis
Copy link
Contributor Author

relates to:
#841

@tbradellis tbradellis self-assigned this Aug 30, 2022
@tbradellis tbradellis changed the title remove obsolete ibm workaround remove obsolete ibm workaround to support Semeru/OpenJ9 versions Aug 31, 2022
@tbradellis tbradellis changed the title remove obsolete ibm workaround to support Semeru/OpenJ9 versions remove obsolete ibm workaround in order to support Semeru/OpenJ9 versions Aug 31, 2022
@tbradellis tbradellis changed the title remove obsolete ibm workaround in order to support Semeru/OpenJ9 versions remove obsolete ibm workaround - support Semeru/OpenJ9 versions Aug 31, 2022
@tbradellis
Copy link
Contributor Author

Related to ait work needed for these runtimes:
actions/setup-java#279
actions/setup-java#289

@@ -98,6 +101,7 @@ public void testNoticedStringDoesNotBubble() {
));
}

@Category(IBMJ9IncompatibleTest.class)
@Test
public void testNoticedErrorOverridesThrownError() {
runTransaction(new SpanErrorFlow.NoticeAndThrow());
Copy link
Contributor Author

@tbradellis tbradellis Sep 1, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IBMj9 throws this as Division by zero and our tests look for the String / by zero.

@@ -72,7 +73,7 @@ public void testMissingNewRelicClass() throws ClassNotFoundException {

// Java 12 no longer lets us access the declared field
@Test
@Category({ Java12IncompatibleTest.class, Java13IncompatibleTest.class, Java14IncompatibleTest.class,
@Category({ IBMJ9IncompatibleTest.class, Java12IncompatibleTest.class, Java13IncompatibleTest.class, Java14IncompatibleTest.class,
Java15IncompatibleTest.class, Java16IncompatibleTest.class, Java17IncompatibleTest.class, Java18IncompatibleTest.class })
public void testSetSystemClassLoader() throws Exception {

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Classloader class used in the J9 implementation does not have the field scl.

@@ -39,7 +40,9 @@
import static org.junit.Assert.assertNotNull;

// Issue when running cassandra unit on Java 9+ - https://github.com/jsevellec/cassandra-unit/issues/249
@Category({Java9IncompatibleTest.class, Java10IncompatibleTest.class, Java11IncompatibleTest.class, Java12IncompatibleTest.class, Java13IncompatibleTest.class, Java14IncompatibleTest.class,
// Many OpenJ9 and Cassandra issues e.g. :https://github.com/jsevellec/cassandra-unit/issues/295
Copy link
Contributor Author

@tbradellis tbradellis Sep 1, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All of these tests failed, but the failures were J9+Cassandra compatability. From what I can tell, anyone attempting to use Cassandra with OpenJ9 is already having issues.

@tbradellis
Copy link
Contributor Author

lasp_strip_exceptions.py test needs to be reworked specifically for OpenJ9. The return value for ArithmeticException is different than openJDK. / by zero vs divide by zero

This will need to be done when the AIT work for IBM Semeru/OpenJ9 is implemented.

@tbradellis
Copy link
Contributor Author

Our current gradle version is blocking us from testing with Semeru. Long story, but we had upgrading our gradle version on the road map anyway.
This will get us to 7.5.1. Gradle will work well with Semeru builds starting in 7.4
#997

@tbradellis
Copy link
Contributor Author

tbradellis commented Sep 6, 2022

errors.py:

--Warning: Unexpected Scoped Metrics--
Did not expect <Error ts:1662248600895
 path:Unknown
 message:divide by zero
 exception_type:java.lang.ArithmeticException
 params:{'userAttributes': {'str': 'dude'}, 'intrinsics': {'error.expected': True}, 'agentAttributes': {'request.uri': ''}, 'stack_trace': ['\tcom.nr.test.servlet.NewRelicApiServlet$1.run(NewRelicApiServlet.java:26)', '\tjava.lang.Thread.run(Thread.java:827)']}>
--/Warning: Unexpected Scoped Metrics--

This is similar to the SpanErrorsTest failure:
#993 (comment)

It looks like OpenJ9 uses the string divide by zero vs / by zero which is the string our tests compare against.

        self.assertIterAttributes(
            self.test_env.collector.get_errors(),
            {
                'path'           : 'Unknown',
                'message'        : '/ by zero',
                'exception_type' : 'java.lang.ArithmeticException'
            })


Various in inf_tracing_test.py

runtest.sh: running test tests/java/functionality/basic_features/inf_tracing_test.py with args 
/home/runner/work/newrelic-java-agent/newrelic-java-agent/agent-integration-tests/src/newrelic/agenttest/testenv.py:295: SyntaxWarning: "is not" with a literal. Did you mean "!="?
  if len(self.exceptions) is not 0:
/home/runner/work/newrelic-java-agent/newrelic-java-agent/agent-integration-tests/src/newrelic/agenttest/testenv.py:323: SyntaxWarning: "is not" with a literal. Did you mean "!="?
  if os.environ.get('TEST_FAIL_EXCEPTIONS', False) and len(self.unique_exceptions) is not 0:

Others as well:
https://github.com/newrelic/newrelic-java-agent/runs/8172652668?check_suite_focus=true

Unit tests for Java 8 and 11 OpenJ9 do not finish. It is unclear why.
https://github.com/newrelic/newrelic-java-agent/runs/8172302024?check_suite_focus=true

@kford-newrelic kford-newrelic added this to Triage in Java Engineering Board via automation Sep 8, 2022
@kford-newrelic kford-newrelic moved this from Triage to Needs Review in Java Engineering Board Sep 8, 2022
@tbradellis tbradellis changed the title remove obsolete ibm workaround - support Semeru/OpenJ9 versions NEWRELIC-3836 remove obsolete ibm workaround - support Semeru/OpenJ9 versions Oct 6, 2022
@tbradellis tbradellis force-pushed the bellis/semeru-pr-branch branch 2 times, most recently from 9196cf2 to ada7129 Compare October 15, 2022 03:09
run tests for 8 and 11 on J9

test exclude openj9 on scl test

try use Ibm sys classloader

expect GeneratedSerializationConstructor

remove -Porg.gradle.java.installations.auto-download=false

get branch current with rebase

exclude cassandra datastax test

add some why comments

add some why comments

add some why comments

fix java install

add incompatible test marker

revert back to temurin
@tbradellis tbradellis merged commit ced4864 into main Oct 17, 2022
Java Engineering Board automation moved this from Needs Review to Done Oct 17, 2022
@tbradellis tbradellis deleted the bellis/semeru-pr-branch branch October 17, 2022 15:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

2 participants