-
Notifications
You must be signed in to change notification settings - Fork 277
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
Apache http client fix exception handling #417
Merged
mar-kolya
merged 12 commits into
master
from
mar-kolya/apache-http-client-fix-exception-handling
Aug 1, 2018
Merged
Changes from all commits
Commits
Show all changes
12 commits
Select commit
Hold shift + click to select a range
b6118f0
Apache HTTP Client: add test for redirected request
mar-kolya 4ae9263
Set POOL_ONLY DescriptionStrategy for ByteBuddy
mar-kolya bae7951
Fix DDLocationStrategy to use DataDog ClassLoader
mar-kolya 1ebe473
Stop using `failSafe` matcher
mar-kolya deec4a7
Improve Apache HTTP client
mar-kolya 6bc1d1a
Implement `saveHasSuperType` and use it instead of `hasSuperType`
mar-kolya 6693a93
Remove onInstrumentationError from LagomTest
mar-kolya c66bd24
Use `safeHasSuperType` instead of `isSubType`
mar-kolya 78e6a9c
Get rid of DDAdvice
mar-kolya 2bfb7b9
Disable Lagom circuit breaker
mar-kolya bb2126b
Improve ByteBuddyElementMatchers javadoc
mar-kolya 7ad9305
Add some integration tests to check ByteBuddy's behavoir on class loa…
mar-kolya File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
8 changes: 0 additions & 8 deletions
8
...tests/src/test/groovy/datadog/trace/agent/integration/classloading/ClassToInstrument.java
This file was deleted.
Oops, something went wrong.
9 changes: 9 additions & 0 deletions
9
dd-java-agent-ittests/src/test/java/datadog/test/ClassToInstrument.java
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,9 @@ | ||
package datadog.test; | ||
|
||
import datadog.trace.api.Trace; | ||
|
||
/** Note: this has to stay in 'datadog.test' package to be considered for instrumentation */ | ||
public class ClassToInstrument { | ||
@Trace | ||
public static void someMethod() {} | ||
} |
4 changes: 4 additions & 0 deletions
4
dd-java-agent-ittests/src/test/java/datadog/test/ClassToInstrumentChild.java
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,4 @@ | ||
package datadog.test; | ||
|
||
/** Note: this has to stay in 'datadog.test' package to be considered for instrumentation */ | ||
public class ClassToInstrumentChild extends ClassToInstrument {} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
143 changes: 143 additions & 0 deletions
143
...ent/agent-tooling/src/main/java/datadog/trace/agent/tooling/ByteBuddyElementMatchers.java
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,143 @@ | ||
package datadog.trace.agent.tooling; | ||
|
||
import static net.bytebuddy.matcher.ElementMatchers.erasure; | ||
|
||
import java.util.HashSet; | ||
import java.util.Set; | ||
import lombok.extern.slf4j.Slf4j; | ||
import net.bytebuddy.build.HashCodeAndEqualsPlugin; | ||
import net.bytebuddy.description.type.TypeDefinition; | ||
import net.bytebuddy.description.type.TypeDescription; | ||
import net.bytebuddy.description.type.TypeList; | ||
import net.bytebuddy.matcher.ElementMatcher; | ||
import net.bytebuddy.matcher.ElementMatchers; | ||
|
||
/** | ||
* This class provides some custom ByteBuddy element matchers to use when applying instrumentation | ||
*/ | ||
@Slf4j | ||
public class ByteBuddyElementMatchers { | ||
|
||
/** | ||
* Matches any type description that declares a super type that matches the provided matcher. | ||
* Exceptions during matching process are logged and ignored. | ||
* | ||
* @param matcher The type to be checked for being a super type of the matched type. | ||
* @param <T> The type of the matched object. | ||
* @return A matcher that matches any type description that declares a super type that matches the | ||
* provided matcher. | ||
* @see ElementMatchers#hasSuperType(net.bytebuddy.matcher.ElementMatcher) | ||
*/ | ||
public static <T extends TypeDescription> ElementMatcher.Junction<T> safeHasSuperType( | ||
final ElementMatcher<? super TypeDescription> matcher) { | ||
return safeHasGenericSuperType(erasure(matcher)); | ||
} | ||
|
||
/** | ||
* Matches any type description that declares a super type that matches the provided matcher. | ||
* Exceptions during matching process are logged and ignored. | ||
* | ||
* @param matcher The type to be checked for being a super type of the matched type. | ||
* @param <T> The type of the matched object. | ||
* @return A matcher that matches any type description that declares a super type that matches the | ||
* provided matcher. | ||
* @see ElementMatchers#hasGenericSuperType(net.bytebuddy.matcher.ElementMatcher) | ||
*/ | ||
public static <T extends TypeDescription> ElementMatcher.Junction<T> safeHasGenericSuperType( | ||
final ElementMatcher<? super TypeDescription.Generic> matcher) { | ||
return new SafeHasSuperTypeMatcher<>(matcher); | ||
} | ||
|
||
/** | ||
* An element matcher that matches a super type. This is different from {@link | ||
* net.bytebuddy.matcher.HasSuperTypeMatcher} in the following way: | ||
* | ||
* <ul> | ||
* <li>Exceptions are logged | ||
* <li>When exception happens the rest of the inheritance subtree is discarded (since ByteBuddy | ||
* cannot load/parse type information for it) but search in other subtrees continues | ||
* </ul> | ||
* | ||
* <p>This is useful because this allows us to see when matcher's check is not complete (i.e. part | ||
* of it fails), at the same time it makes best effort instead of failing quickly (like {@code | ||
* failSafe(hasSuperType(...))} does) which means the code is more resilient to classpath | ||
* inconsistencies | ||
* | ||
* @param <T> The type of the matched entity. | ||
* @see net.bytebuddy.matcher.HasSuperTypeMatcher | ||
*/ | ||
@HashCodeAndEqualsPlugin.Enhance | ||
public static class SafeHasSuperTypeMatcher<T extends TypeDescription> | ||
extends ElementMatcher.Junction.AbstractBase<T> { | ||
|
||
/** The matcher to apply to any super type of the matched type. */ | ||
private final ElementMatcher<? super TypeDescription.Generic> matcher; | ||
|
||
/** | ||
* Creates a new matcher for a super type. | ||
* | ||
* @param matcher The matcher to apply to any super type of the matched type. | ||
*/ | ||
public SafeHasSuperTypeMatcher(final ElementMatcher<? super TypeDescription.Generic> matcher) { | ||
this.matcher = matcher; | ||
} | ||
|
||
@Override | ||
public boolean matches(final T target) { | ||
final Set<TypeDescription> checkedInterfaces = new HashSet<>(); | ||
// We do not use foreach loop and iterator interface here because we need to catch exceptions | ||
// in {@code getSuperClass} calls | ||
TypeDefinition typeDefinition = target; | ||
while (typeDefinition != null) { | ||
if (matcher.matches(typeDefinition.asGenericType()) | ||
|| hasInterface(typeDefinition, checkedInterfaces)) { | ||
return true; | ||
} | ||
typeDefinition = safeGetSuperClass(typeDefinition); | ||
} | ||
return false; | ||
} | ||
|
||
private TypeDefinition safeGetSuperClass(final TypeDefinition typeDefinition) { | ||
try { | ||
return typeDefinition.getSuperClass(); | ||
} catch (final Exception e) { | ||
log.info("Exception trying to get next type definition:", e); | ||
return null; | ||
} | ||
} | ||
|
||
/** | ||
* Matches a type's interfaces against the provided matcher. | ||
* | ||
* @param typeDefinition The type for which to check all implemented interfaces. | ||
* @param checkedInterfaces The interfaces that have already been checked. | ||
* @return {@code true} if any interface matches the supplied matcher. | ||
*/ | ||
private boolean hasInterface( | ||
final TypeDefinition typeDefinition, final Set<TypeDescription> checkedInterfaces) { | ||
for (final TypeDefinition interfaceType : safeGetInterfaces(typeDefinition)) { | ||
if (checkedInterfaces.add(interfaceType.asErasure()) | ||
&& (matcher.matches(interfaceType.asGenericType()) | ||
|| hasInterface(interfaceType, checkedInterfaces))) { | ||
return true; | ||
} | ||
} | ||
return false; | ||
} | ||
|
||
private TypeList.Generic safeGetInterfaces(final TypeDefinition typeDefinition) { | ||
try { | ||
return typeDefinition.getInterfaces(); | ||
} catch (final Exception e) { | ||
log.info("Exception trying to get interfaces:", e); | ||
return new TypeList.Generic.Empty(); | ||
} | ||
} | ||
|
||
@Override | ||
public String toString() { | ||
return "safeHasSuperType(" + matcher + ")"; | ||
} | ||
} | ||
} |
33 changes: 0 additions & 33 deletions
33
dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/DDAdvice.java
This file was deleted.
Oops, something went wrong.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you include in your comment how this is different from the
SuperTypeMatcher
that is built into byte buddy?Also describe how this is different from using
failSafe(hasSuperType(...))
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should be fixed now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
much better.