Ignore appdynamics/cisco multi tenant agent module #326
Conversation
pavolloffay
changed the title
| @@ -55,6 +81,16 @@ public Result classloader(ClassLoader classLoader) { | |||
| if (name.startsWith("com.singularity.") || name.startsWith("com.yourkit.")) { | |||
| return Result.IGNORE; | |||
| } | |||
| if (CLASS_LOADER_GET_NAME != null) { | |||
There was a problem hiding this comment.
Can you explain why testing against name does not work?
What is the name when the getName returns MultiTenantAgentClassLoader
There was a problem hiding this comment.
Based on the log messages, I tried using the name, but I thought that the class loader wasn't namespaced in a cisco package so my first pass at this solution was adding the check "MultiTenantAgentClassLoader".equals(name) to the if(..) condition above. I was surprised when it didn't work, so I went with this one. Turns out, the class name is namedspaced properly and is com.cisco.mtagent.entry.MTAgent$MultiTenantAgentClassLoader so we definitely don't need all of this extra complexity. I'm very happy to simply check for if the class loader class name starts with com.cisco. and being more comortable knowing this should be relatively stable over time 😄 Thanks for asking, much happier with the solution we landed on as a result!
There was a problem hiding this comment.
Again final keyword in the places where it does not make any sense.
I would like to understand why you are not using the existing variable name that has the class name. Also the fact that getName is java 9+ makes this more fragile and there should be a way to do it regardless the java version.
|
Circling back to using the |
|
Re: the |
| if (name.startsWith("com.singularity.") || name.startsWith("com.yourkit.")) { | ||
| if (name.startsWith("com.singularity.") | ||
| || name.startsWith("com.yourkit.") | ||
| || name.startsWith("com.cisco.")) { |
There was a problem hiding this comment.
com.cisco is too restrictive. Imagine someone from cisco using this agent. Look at the upstream class how it excluded new relic classes - the same story they wanted to use the agent internally so the agent had to exclude only NR agent classes.
There was a problem hiding this comment.
Good point! I'll update this to be com.cisco.mtagent. to match the com.cisco.mtagent.entry.MTAgent$MultiTenantAgentClassLoader
I encourage anybody to have a look, it's a great learning experience on how to write quality code. |
I haven't seen a code style check that would do anything with |
ryandens
force-pushed
the
appdynamics-classloader
branch
from
941ed55 to
9c1254d
Compare
|
cisco is going to be the top company in classloader exclusions (appd, singularity, now cisco). |
This is a good point - @ryandens can you scope the package down? |
Yep, this is already done! I scoped it down to |
Description
When running the Hypertrace Javaagent with the Appdynamics javaagent on 9+ JVM, the following error occurs after ~3 minutes of leaving an app server running with no HTTP traffic
Testing
I verified this locally using Tomcat 9 on Correto 11 with Hypertrace and Appdynamics installed.
Checklist:
Documentation
Make sure that you have documented corresponding changes in this repository or hypertrace docs repo if required.