Skip to content

Commit b751d37

Browse files
steveraosteverao
committed
Address review comments
1 parent 0c4aabe commit b751d37

File tree

6 files changed

+72
-58
lines changed

6 files changed

+72
-58
lines changed

‎instrumentation/apache-elasticjob-3.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/apacheelasticjob/v3_0/DataflowJobExecutorInstrumentation.java‎

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@
88
import static io.opentelemetry.javaagent.instrumentation.apacheelasticjob.v3_0.ElasticJobSingletons.helper;
99
import static net.bytebuddy.matcher.ElementMatchers.isMethod;
1010
import static net.bytebuddy.matcher.ElementMatchers.named;
11-
import static net.bytebuddy.matcher.ElementMatchers.takesArguments;
11+
import static net.bytebuddy.matcher.ElementMatchers.takesArgument;
1212

1313
import io.opentelemetry.javaagent.extension.instrumentation.TypeInstrumentation;
1414
import io.opentelemetry.javaagent.extension.instrumentation.TypeTransformer;
@@ -29,7 +29,10 @@ public ElementMatcher<TypeDescription> typeMatcher() {
2929
@Override
3030
public void transform(TypeTransformer transformer) {
3131
transformer.applyAdviceToMethod(
32-
isMethod().and(named("process")).and(takesArguments(4)),
32+
isMethod()
33+
.and(named("process"))
34+
.and(takesArgument(0, named("org.apache.shardingsphere.elasticjob.dataflow.job.DataflowJob")))
35+
.and(takesArgument(3, named("org.apache.shardingsphere.elasticjob.api.ShardingContext"))),
3336
DataflowJobExecutorInstrumentation.class.getName() + "$ProcessAdvice");
3437
}
3538

‎instrumentation/apache-elasticjob-3.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/apacheelasticjob/v3_0/ElasticJobExecutorInstrumentation.java‎

Lines changed: 1 addition & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@
66
package io.opentelemetry.javaagent.instrumentation.apacheelasticjob.v3_0;
77

88
import static io.opentelemetry.javaagent.instrumentation.apacheelasticjob.v3_0.ElasticJobSingletons.helper;
9+
import static io.opentelemetry.javaagent.instrumentation.apacheelasticjob.v3_0.JobTypeHelper.determineJobTypeFromExecutor;
910
import static net.bytebuddy.matcher.ElementMatchers.named;
1011
import static net.bytebuddy.matcher.ElementMatchers.takesArgument;
1112

@@ -69,25 +70,6 @@ public static ElasticJobHelper.ElasticJobScope onEnter(
6970
return helper().startSpan(request);
7071
}
7172

72-
public static String determineJobTypeFromExecutor(Object jobItemExecutor) {
73-
if (jobItemExecutor == null) {
74-
return "UNKNOWN";
75-
} else {
76-
switch (jobItemExecutor.getClass().getSimpleName()) {
77-
case "HttpJobExecutor":
78-
return "HTTP";
79-
case "ScriptJobExecutor":
80-
return "SCRIPT";
81-
case "SimpleJobExecutor":
82-
return "SIMPLE";
83-
case "DataflowJobExecutor":
84-
return "DATAFLOW";
85-
default:
86-
return "UNKNOWN";
87-
}
88-
}
89-
}
90-
9173
@Advice.OnMethodExit(onThrowable = Throwable.class, suppress = Throwable.class)
9274
public static void onExit(
9375
@Advice.Enter @Nullable ElasticJobHelper.ElasticJobScope scope,

‎instrumentation/apache-elasticjob-3.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/apacheelasticjob/v3_0/ElasticJobHelper.java‎

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -34,10 +34,6 @@ public ElasticJobScope startSpan(ElasticJobProcessRequest request) {
3434

3535
public void endSpan(@Nullable ElasticJobScope scope, @Nullable Throwable throwable) {
3636
if (scope != null) {
37-
if (throwable != null) {
38-
scope.request.setFailed();
39-
}
40-
4137
scope.scope.close();
4238
this.instrumenter.end(scope.context, scope.request, null, throwable);
4339
}

‎instrumentation/apache-elasticjob-3.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/apacheelasticjob/v3_0/JobTypeHelper.java‎

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,31 @@
1+
/*
2+
* Copyright The OpenTelemetry Authors
3+
* SPDX-License-Identifier: Apache-2.0
4+
*/
5+
6+
package io.opentelemetry.javaagent.instrumentation.apacheelasticjob.v3_0;
7+
8+
public final class JobTypeHelper {
9+
10+
public static String determineJobTypeFromExecutor(Object jobItemExecutor) {
11+
if (jobItemExecutor == null) {
12+
return "UNKNOWN";
13+
} else {
14+
switch (jobItemExecutor.getClass().getSimpleName()) {
15+
case "HttpJobExecutor":
16+
return "HTTP";
17+
case "ScriptJobExecutor":
18+
return "SCRIPT";
19+
case "SimpleJobExecutor":
20+
return "SIMPLE";
21+
case "DataflowJobExecutor":
22+
return "DATAFLOW";
23+
default:
24+
return "UNKNOWN";
25+
}
26+
}
27+
}
28+
29+
private JobTypeHelper() {}
30+
}
31+

‎instrumentation/apache-elasticjob-3.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/apacheelasticjob/v3_0/SimpleJobExecutorInstrumentation.java‎

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@
88
import static io.opentelemetry.javaagent.instrumentation.apacheelasticjob.v3_0.ElasticJobSingletons.helper;
99
import static net.bytebuddy.matcher.ElementMatchers.isMethod;
1010
import static net.bytebuddy.matcher.ElementMatchers.named;
11-
import static net.bytebuddy.matcher.ElementMatchers.takesArguments;
11+
import static net.bytebuddy.matcher.ElementMatchers.takesArgument;
1212

1313
import io.opentelemetry.javaagent.extension.instrumentation.TypeInstrumentation;
1414
import io.opentelemetry.javaagent.extension.instrumentation.TypeTransformer;
@@ -29,7 +29,10 @@ public ElementMatcher<TypeDescription> typeMatcher() {
2929
@Override
3030
public void transform(TypeTransformer transformer) {
3131
transformer.applyAdviceToMethod(
32-
isMethod().and(named("process")).and(takesArguments(4)),
32+
isMethod()
33+
.and(named("process"))
34+
.and(takesArgument(0, named("org.apache.shardingsphere.elasticjob.simple.job.SimpleJob")))
35+
.and(takesArgument(3, named("org.apache.shardingsphere.elasticjob.api.ShardingContext"))),
3336
SimpleJobExecutorInstrumentation.class.getName() + "$ProcessAdvice");
3437
}
3538

‎instrumentation/apache-elasticjob-3.0/javaagent/src/test/java/io/opentelemetry/javaagent/instrumentation/apacheelasticjob/v3_0/ElasticJobTest.java‎

Lines changed: 30 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -46,6 +46,8 @@
4646
import org.junit.jupiter.api.AfterEach;
4747
import org.junit.jupiter.api.BeforeAll;
4848
import org.junit.jupiter.api.Test;
49+
import org.junit.jupiter.api.condition.DisabledOnOs;
50+
import org.junit.jupiter.api.condition.OS;
4951
import org.junit.jupiter.api.extension.RegisterExtension;
5052

5153
public class ElasticJobTest {
@@ -87,8 +89,6 @@ public static void stop() {
8789

8890
@AfterEach
8991
public void clearSpans() {
90-
testing.clearData();
91-
9292
try {
9393
Thread.sleep(100L);
9494
} catch (InterruptedException var2) {
@@ -97,7 +97,7 @@ public void clearSpans() {
9797
}
9898

9999
@Test
100-
public void testHttpJob() throws InterruptedException {
100+
public void testHttpJob() {
101101
ScheduleJobBootstrap bootstrap = setUpHttpJob(regCenter);
102102
try {
103103
bootstrap.schedule();
@@ -273,38 +273,37 @@ public void testDataflowJob() throws InterruptedException {
273273
}
274274

275275
@Test
276+
@DisabledOnOs(OS.WINDOWS)
276277
public void testScriptJob() throws IOException {
277278
ScheduleJobBootstrap bootstrap = setUpScriptJob(regCenter);
278279
try {
279-
testing.waitAndAssertTracesWithoutScopeVersionVerification(
280+
testing.waitAndAssertTraces(
280281
trace ->
281-
trace
282-
.hasSize(1)
283-
.hasSpansSatisfyingExactly(
284-
span ->
285-
span.hasKind(SpanKind.INTERNAL)
286-
.hasName("SCRIPT")
287-
.hasAttributesSatisfyingExactly(
288-
equalTo(AttributeKey.stringKey("job.system"), "elasticjob"),
289-
equalTo(
290-
AttributeKey.stringKey(
291-
"scheduling.apache-elasticjob.job.name"),
292-
"scriptElasticJob"),
293-
equalTo(
294-
AttributeKey.longKey("scheduling.apache-elasticjob.item"),
295-
0L),
296-
equalTo(
297-
AttributeKey.longKey(
298-
"scheduling.apache-elasticjob.sharding.total.count"),
299-
1L),
300-
equalTo(
301-
AttributeKey.stringKey(
302-
"scheduling.apache-elasticjob.sharding.item.parameters"),
303-
"{0=null}"),
304-
satisfies(
305-
AttributeKey.stringKey(
306-
"scheduling.apache-elasticjob.task.id"),
307-
taskId -> taskId.contains("scriptElasticJob")))));
282+
trace.hasSpansSatisfyingExactly(
283+
span ->
284+
span.hasKind(SpanKind.INTERNAL)
285+
.hasName("SCRIPT")
286+
.hasAttributesSatisfyingExactly(
287+
equalTo(AttributeKey.stringKey("job.system"), "elasticjob"),
288+
equalTo(
289+
AttributeKey.stringKey(
290+
"scheduling.apache-elasticjob.job.name"),
291+
"scriptElasticJob"),
292+
equalTo(
293+
AttributeKey.longKey("scheduling.apache-elasticjob.item"),
294+
0L),
295+
equalTo(
296+
AttributeKey.longKey(
297+
"scheduling.apache-elasticjob.sharding.total.count"),
298+
1L),
299+
equalTo(
300+
AttributeKey.stringKey(
301+
"scheduling.apache-elasticjob.sharding.item.parameters"),
302+
"{0=null}"),
303+
satisfies(
304+
AttributeKey.stringKey(
305+
"scheduling.apache-elasticjob.task.id"),
306+
taskId -> taskId.contains("scriptElasticJob")))));
308307
} finally {
309308
bootstrap.shutdown();
310309
}

0 commit comments

Comments
 (0)