-
Notifications
You must be signed in to change notification settings - Fork 335
[DO NOT MERGE] feat(jedis): Add jedis-gen-3.0 module + test-framework bug fixes (toolkit-generated) #11338
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
base: master
Are you sure you want to change the base?
[DO NOT MERGE] feat(jedis): Add jedis-gen-3.0 module + test-framework bug fixes (toolkit-generated) #11338
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,6 +5,7 @@ | |
| import static datadog.trace.agent.test.assertions.Matchers.isNonNull; | ||
| import static datadog.trace.api.DDTags.BASE_SERVICE; | ||
| import static datadog.trace.api.DDTags.DD_INTEGRATION; | ||
| import static datadog.trace.api.DDTags.DD_SVC_SRC; | ||
| import static datadog.trace.api.DDTags.DJM_ENABLED; | ||
| import static datadog.trace.api.DDTags.DSM_ENABLED; | ||
| import static datadog.trace.api.DDTags.ERROR_MSG; | ||
|
|
@@ -54,6 +55,7 @@ public static TagsMatcher defaultTags() { | |
| tagMatchers.put(PARENT_ID, any()); | ||
| tagMatchers.put(SPAN_LINKS, any()); // this is checked by LinksAsserter | ||
| tagMatchers.put(DD_INTEGRATION, any()); | ||
| tagMatchers.put(DD_SVC_SRC, any()); | ||
| tagMatchers.put(TRACER_HOST, any()); | ||
|
|
||
| for (String tagName : REQUIRED_CODE_ORIGIN_TAGS) { | ||
|
|
@@ -127,6 +129,8 @@ public static TagsMatcher error(Class<? extends Throwable> errorType, String mes | |
| tagMatchers.put(ERROR_STACK, isNonNull()); | ||
| if (message != null) { | ||
| tagMatchers.put(ERROR_MSG, is(message)); | ||
| } else { | ||
| tagMatchers.put(ERROR_MSG, any()); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why this is needed? That's changes the tag assertion behaviour for all the assertions. A change on it must be justified and commented with a rationale |
||
| } | ||
| return new TagsMatcher(tagMatchers); | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,38 @@ | ||
| muzzle { | ||
| fail { | ||
| group = "redis.clients" | ||
| module = "jedis" | ||
| versions = "[,3.0.0)" | ||
| skipVersions += "jedis-3.6.2" // bad release version ("jedis-" prefix) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is the exact same comment we have in the original jedis-3. @jordan-wong , are we sure the toolkit isn’t being influenced by existing context or something similar? If it had no prior knowledge of the module, it shouldn’t be able to reproduce the exact same muzzle directives and comments. |
||
| } | ||
|
|
||
| pass { | ||
| group = "redis.clients" | ||
| module = "jedis" | ||
| versions = "[3.0.0,4.0.0)" | ||
| } | ||
|
|
||
| // Upper bound (jedis 4.0+) is enforced by jedis-4.0 module's instrumentation; | ||
| // its own fail{ versions = "[,4.0.0)" } guards against this module loading on 4.0+. | ||
|
Comment on lines
+15
to
+16
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this comment is misleading. This does not prevent this module from being loaded. The jedis-4 module tests the lower bound (i.e. muzzle deactivates the advices if < 4) but here we are not testing that muzzle fails on >=4. So either this is not tested and can be added either muzzle passes because the advice does not applies at all (so it's fine) but this needs to be documented |
||
| } | ||
|
|
||
| apply from: "$rootDir/gradle/java.gradle" | ||
|
|
||
| addTestSuiteForDir('latestDepTest', 'test') | ||
|
|
||
| tasks.withType(Test).configureEach { | ||
| environment "DD_TRACE_ENABLED", "true" | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this should not be there nor needed. It's already enabled by default |
||
| } | ||
|
|
||
| dependencies { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. also including previous versions' instrumentation for the same library ensure that they don't overlap in tests (i.e. their activation must be mutually exclusive) otherwise there is the risk to double instrument the library and this should be also tested |
||
| compileOnly group: 'redis.clients', name: 'jedis', version: '3.0.0' | ||
| testImplementation group: 'redis.clients', name: 'jedis', version: '3.0.0' | ||
|
|
||
| testImplementation (group: 'com.github.codemonstur', name: 'embedded-redis', version: '1.4.3') { | ||
| // Excluding redis client to avoid conflicts in instrumentation code. | ||
| exclude group: 'redis.clients', module: 'jedis' | ||
| } | ||
|
|
||
| // Jedis 4.0 has API changes that prevent this instrumentation from applying | ||
| latestDepTestImplementation group: 'redis.clients', name: 'jedis', version: '3.+' | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,70 @@ | ||
| package datadog.trace.instrumentation.jedis3; | ||
|
|
||
| import datadog.trace.api.naming.SpanNaming; | ||
| import datadog.trace.bootstrap.instrumentation.api.AgentSpan; | ||
| import datadog.trace.bootstrap.instrumentation.api.InternalSpanTypes; | ||
| import datadog.trace.bootstrap.instrumentation.api.UTF8BytesString; | ||
| import datadog.trace.bootstrap.instrumentation.decorator.DBTypeProcessingDatabaseClientDecorator; | ||
| import redis.clients.jedis.BinaryClient; | ||
| import redis.clients.jedis.Connection; | ||
|
|
||
| public class JedisClientDecorator extends DBTypeProcessingDatabaseClientDecorator<Connection> { | ||
| private static final String REDIS = "redis"; | ||
| public static final CharSequence COMPONENT_NAME = UTF8BytesString.create("redis-command"); | ||
| public static final CharSequence OPERATION_NAME = | ||
| UTF8BytesString.create(SpanNaming.instance().namingSchema().cache().operation(REDIS)); | ||
| private static final String SERVICE_NAME = | ||
| SpanNaming.instance().namingSchema().cache().service(REDIS); | ||
| public static final JedisClientDecorator DECORATE = new JedisClientDecorator(); | ||
|
|
||
| @Override | ||
| protected String[] instrumentationNames() { | ||
| return new String[] {"jedis", REDIS}; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. usually we also add a name that contains the version (i.e. |
||
| } | ||
|
|
||
| @Override | ||
| protected String service() { | ||
| return SERVICE_NAME; | ||
| } | ||
|
|
||
| @Override | ||
| protected CharSequence component() { | ||
| return COMPONENT_NAME; | ||
| } | ||
|
|
||
| @Override | ||
| protected CharSequence spanType() { | ||
| return InternalSpanTypes.REDIS; | ||
| } | ||
|
|
||
| @Override | ||
| protected String dbType() { | ||
| return REDIS; | ||
| } | ||
|
|
||
| @Override | ||
| protected String dbUser(final Connection connection) { | ||
| return null; | ||
| } | ||
|
|
||
| @Override | ||
| protected String dbInstance(final Connection connection) { | ||
| return null; | ||
| } | ||
|
|
||
| @Override | ||
| protected String dbHostname(Connection connection) { | ||
| return connection.getHost(); | ||
| } | ||
|
|
||
| @Override | ||
| public AgentSpan onConnection(final AgentSpan span, final Connection connection) { | ||
| if (connection != null) { | ||
| setPeerPort(span, connection.getPort()); | ||
| if (connection instanceof BinaryClient) { | ||
| span.setTag("db.redis.dbIndex", ((BinaryClient) connection).getDB()); | ||
| } | ||
|
Comment on lines
+64
to
+66
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This codepath looks untested |
||
| } | ||
| return super.onConnection(span, connection); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,94 @@ | ||
| package datadog.trace.instrumentation.jedis3; | ||
|
|
||
| import static datadog.trace.agent.tooling.bytebuddy.matcher.ClassLoaderMatchers.hasClassNamed; | ||
| import static datadog.trace.agent.tooling.bytebuddy.matcher.NameMatchers.named; | ||
| import static datadog.trace.bootstrap.instrumentation.api.AgentTracer.activateSpan; | ||
| import static datadog.trace.bootstrap.instrumentation.api.AgentTracer.startSpan; | ||
| import static datadog.trace.instrumentation.jedis3.JedisClientDecorator.COMPONENT_NAME; | ||
| import static datadog.trace.instrumentation.jedis3.JedisClientDecorator.DECORATE; | ||
| import static net.bytebuddy.matcher.ElementMatchers.isMethod; | ||
| import static net.bytebuddy.matcher.ElementMatchers.not; | ||
| import static net.bytebuddy.matcher.ElementMatchers.takesArgument; | ||
|
|
||
| import com.google.auto.service.AutoService; | ||
| import datadog.trace.agent.tooling.Instrumenter; | ||
| import datadog.trace.agent.tooling.InstrumenterModule; | ||
| import datadog.trace.bootstrap.CallDepthThreadLocalMap; | ||
| import datadog.trace.bootstrap.instrumentation.api.AgentScope; | ||
| import datadog.trace.bootstrap.instrumentation.api.AgentSpan; | ||
| import net.bytebuddy.asm.Advice; | ||
| import net.bytebuddy.matcher.ElementMatcher; | ||
| import redis.clients.jedis.Connection; | ||
| import redis.clients.jedis.Protocol; | ||
| import redis.clients.jedis.commands.ProtocolCommand; | ||
|
|
||
| @AutoService(InstrumenterModule.class) | ||
| public final class JedisInstrumentation extends InstrumenterModule.Tracing | ||
| implements Instrumenter.ForSingleType, Instrumenter.HasMethodAdvice { | ||
|
|
||
| public JedisInstrumentation() { | ||
| super("jedis", "redis"); | ||
| } | ||
|
|
||
| @Override | ||
| public ElementMatcher.Junction<ClassLoader> classLoaderMatcher() { | ||
| // Match Jedis 3.x which has ProtocolCommand but not CommandObject (introduced in 4.0) | ||
| return hasClassNamed("redis.clients.jedis.commands.ProtocolCommand") | ||
| .and(not(hasClassNamed("redis.clients.jedis.CommandObject"))); | ||
| } | ||
|
|
||
| @Override | ||
| public String instrumentedType() { | ||
| return "redis.clients.jedis.Connection"; | ||
| } | ||
|
|
||
| @Override | ||
| public String[] helperClassNames() { | ||
| return new String[] { | ||
| packageName + ".JedisClientDecorator", | ||
| }; | ||
| } | ||
|
|
||
| @Override | ||
| public void methodAdvice(MethodTransformer transformer) { | ||
| transformer.applyAdvice( | ||
| isMethod() | ||
| .and(named("sendCommand")) | ||
| .and(takesArgument(0, named("redis.clients.jedis.commands.ProtocolCommand"))), | ||
| JedisInstrumentation.class.getName() + "$JedisAdvice"); | ||
| } | ||
|
|
||
| public static class JedisAdvice { | ||
|
|
||
| @Advice.OnMethodEnter(suppress = Throwable.class) | ||
| public static AgentScope onEnter( | ||
| @Advice.Argument(0) final ProtocolCommand command, @Advice.This final Connection thiz) { | ||
| if (CallDepthThreadLocalMap.incrementCallDepth(Connection.class) > 0) { | ||
| return null; | ||
| } | ||
| final AgentSpan span = | ||
| startSpan(COMPONENT_NAME.toString(), JedisClientDecorator.OPERATION_NAME); | ||
| DECORATE.afterStart(span); | ||
| DECORATE.onConnection(span, thiz); | ||
| if (command instanceof Protocol.Command) { | ||
| DECORATE.onStatement(span, ((Protocol.Command) command).name()); | ||
| } else { | ||
| DECORATE.onStatement(span, new String(command.getRaw())); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: the new String(byte[]) is using the default charset. While this is in practice harmless for this kind of use case (i.e. redis commands are ASCII) I would always fix the charset to use in the encoding |
||
| } | ||
| return activateSpan(span); | ||
| } | ||
|
|
||
| @Advice.OnMethodExit(onThrowable = Throwable.class, suppress = Throwable.class) | ||
| public static void stopSpan( | ||
| @Advice.Enter final AgentScope scope, @Advice.Thrown final Throwable throwable) { | ||
| if (scope == null) { | ||
| return; | ||
| } | ||
| CallDepthThreadLocalMap.reset(Connection.class); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We use this pattern widely across the codebase, but resetting in |
||
| DECORATE.onError(scope.span(), throwable); | ||
| DECORATE.beforeFinish(scope.span()); | ||
| scope.close(); | ||
| scope.span().finish(); | ||
| } | ||
| } | ||
| } | ||
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.
this is not symmetric (i.e. if this is a Charsequence and this.expected is a String) so the other half looks missing to me