From f8c887e241cabb2d36d68ed1b361cbddf21ed108 Mon Sep 17 00:00:00 2001 From: Brandon Fergerson Date: Sat, 5 Sep 2020 19:14:40 +0400 Subject: [PATCH] Include request method type in rest mapping annotations (#5085) --- .../v4/RestMappingMethodInterceptorTest.java | 20 +++++++++---------- .../RestMappingMethodInterceptor.java | 16 ++++++++++++++- .../skywalking/e2e/kafka/KafkaProfileE2E.java | 5 ++--- .../skywalking/e2e/profile/ProfileE2E.java | 2 +- .../resources/expected/profile/endpoints.yml | 4 ++-- .../resources/expected/profile/finished.yml | 2 +- .../resources/expected/profile/notified.yml | 2 +- .../expected/profile/profileSegment.yml | 2 +- .../expected/profile/profileSegments.yml | 2 +- .../config/expectedData.yaml | 2 +- .../config/expectedData.yaml | 6 +++--- .../config/expectedData.yaml | 10 +++++----- .../config/expectedData.yaml | 2 +- 13 files changed, 44 insertions(+), 31 deletions(-) diff --git a/apm-sniffer/apm-sdk-plugin/spring-plugins/mvc-annotation-4.x-plugin/src/test/java/org/apache/skywalking/apm/plugin/spring/mvc/v4/RestMappingMethodInterceptorTest.java b/apm-sniffer/apm-sdk-plugin/spring-plugins/mvc-annotation-4.x-plugin/src/test/java/org/apache/skywalking/apm/plugin/spring/mvc/v4/RestMappingMethodInterceptorTest.java index 86a1858d5..d30036078 100644 --- a/apm-sniffer/apm-sdk-plugin/spring-plugins/mvc-annotation-4.x-plugin/src/test/java/org/apache/skywalking/apm/plugin/spring/mvc/v4/RestMappingMethodInterceptorTest.java +++ b/apm-sniffer/apm-sdk-plugin/spring-plugins/mvc-annotation-4.x-plugin/src/test/java/org/apache/skywalking/apm/plugin/spring/mvc/v4/RestMappingMethodInterceptorTest.java @@ -147,7 +147,7 @@ public class RestMappingMethodInterceptorTest { TraceSegment traceSegment = segmentStorage.getTraceSegments().get(0); List spans = SegmentHelper.getSpans(traceSegment); - assertHttpSpan(spans.get(0), "/getRequestURL"); + assertHttpSpan(spans.get(0), "{GET}", "/getRequestURL"); } @Test @@ -173,7 +173,7 @@ public class RestMappingMethodInterceptorTest { TraceSegment traceSegment = segmentStorage.getTraceSegments().get(0); List spans = SegmentHelper.getSpans(traceSegment); - assertHttpSpan(spans.get(0), "/postRequestURL"); + assertHttpSpan(spans.get(0), "{POST}", "/postRequestURL"); } @Test @@ -199,7 +199,7 @@ public class RestMappingMethodInterceptorTest { TraceSegment traceSegment = segmentStorage.getTraceSegments().get(0); List spans = SegmentHelper.getSpans(traceSegment); - assertHttpSpan(spans.get(0), "/putRequestURL"); + assertHttpSpan(spans.get(0), "{PUT}", "/putRequestURL"); } @Test @@ -226,7 +226,7 @@ public class RestMappingMethodInterceptorTest { TraceSegment traceSegment = segmentStorage.getTraceSegments().get(0); List spans = SegmentHelper.getSpans(traceSegment); - assertHttpSpan(spans.get(0), "/deleteRequestURL"); + assertHttpSpan(spans.get(0), "{DELETE}", "/deleteRequestURL"); } @Test @@ -252,7 +252,7 @@ public class RestMappingMethodInterceptorTest { TraceSegment traceSegment = segmentStorage.getTraceSegments().get(0); List spans = SegmentHelper.getSpans(traceSegment); - assertHttpSpan(spans.get(0), "/patchRequestURL"); + assertHttpSpan(spans.get(0), "{PATCH}", "/patchRequestURL"); } @Test @@ -278,7 +278,7 @@ public class RestMappingMethodInterceptorTest { TraceSegment traceSegment = segmentStorage.getTraceSegments().get(0); List spans = SegmentHelper.getSpans(traceSegment); - assertHttpSpan(spans.get(0), ""); + assertHttpSpan(spans.get(0), "", ""); } @Test @@ -304,7 +304,7 @@ public class RestMappingMethodInterceptorTest { TraceSegment traceSegment = segmentStorage.getTraceSegments().get(0); List spans = SegmentHelper.getSpans(traceSegment); - assertHttpSpan(spans.get(0), "/getRequestURL"); + assertHttpSpan(spans.get(0), "{GET}", "/getRequestURL"); List logDataEntities = SpanHelper.getLogs(spans.get(0)); assertThat(logDataEntities.size(), is(1)); SpanAssert.assertException(logDataEntities.get(0), RuntimeException.class); @@ -335,7 +335,7 @@ public class RestMappingMethodInterceptorTest { TraceSegment traceSegment = segmentStorage.getTraceSegments().get(0); List spans = SegmentHelper.getSpans(traceSegment); - assertHttpSpan(spans.get(0), "/getRequestURL"); + assertHttpSpan(spans.get(0), "{GET}", "/getRequestURL"); SpanAssert.assertTag(spans.get(0), 2, "connection=[keep-alive]"); } @@ -345,8 +345,8 @@ public class RestMappingMethodInterceptorTest { MatcherAssert.assertThat(SegmentRefHelper.getTraceSegmentId(ref).toString(), is("1.444.555")); } - private void assertHttpSpan(AbstractTracingSpan span, String suffix) { - assertThat(span.getOperationName(), is("/test" + suffix)); + private void assertHttpSpan(AbstractTracingSpan span, String prefix, String suffix) { + assertThat(span.getOperationName(), is(prefix + "/test" + suffix)); SpanAssert.assertComponent(span, ComponentsDefine.SPRING_MVC_ANNOTATION); SpanAssert.assertTag(span, 0, "http://localhost:8080/test" + suffix); assertThat(span.isEntry(), is(true)); diff --git a/apm-sniffer/apm-sdk-plugin/spring-plugins/mvc-annotation-commons/src/main/java/org/apache/skywalking/apm/plugin/spring/mvc/commons/interceptor/RestMappingMethodInterceptor.java b/apm-sniffer/apm-sdk-plugin/spring-plugins/mvc-annotation-commons/src/main/java/org/apache/skywalking/apm/plugin/spring/mvc/commons/interceptor/RestMappingMethodInterceptor.java index 3bb3a1da3..8bd6dfde2 100644 --- a/apm-sniffer/apm-sdk-plugin/spring-plugins/mvc-annotation-commons/src/main/java/org/apache/skywalking/apm/plugin/spring/mvc/commons/interceptor/RestMappingMethodInterceptor.java +++ b/apm-sniffer/apm-sdk-plugin/spring-plugins/mvc-annotation-commons/src/main/java/org/apache/skywalking/apm/plugin/spring/mvc/commons/interceptor/RestMappingMethodInterceptor.java @@ -80,6 +80,20 @@ public class RestMappingMethodInterceptor extends AbstractMethodInterceptor { @Override public String getAcceptedMethodTypes(Method method) { - return ""; + return ParsePathUtil.recursiveParseMethodAnnotation(method, m -> { + if (AnnotationUtils.getAnnotation(m, GetMapping.class) != null) { + return "{GET}"; + } else if (AnnotationUtils.getAnnotation(m, PostMapping.class) != null) { + return "{POST}"; + } else if (AnnotationUtils.getAnnotation(m, PutMapping.class) != null) { + return "{PUT}"; + } else if (AnnotationUtils.getAnnotation(m, DeleteMapping.class) != null) { + return "{DELETE}"; + } else if (AnnotationUtils.getAnnotation(m, PatchMapping.class) != null) { + return "{PATCH}"; + } else { + return null; + } + }); } } diff --git a/test/e2e/e2e-test/src/test/java/org/apache/skywalking/e2e/kafka/KafkaProfileE2E.java b/test/e2e/e2e-test/src/test/java/org/apache/skywalking/e2e/kafka/KafkaProfileE2E.java index f6b33a3d7..cf53fc05f 100644 --- a/test/e2e/e2e-test/src/test/java/org/apache/skywalking/e2e/kafka/KafkaProfileE2E.java +++ b/test/e2e/e2e-test/src/test/java/org/apache/skywalking/e2e/kafka/KafkaProfileE2E.java @@ -141,9 +141,8 @@ public class KafkaProfileE2E extends SkyWalkingTestAdapter { @Order(3) void createProfileTask() throws Exception { final ProfileTaskCreationRequest creationRequest = ProfileTaskCreationRequest.builder() - .serviceId( - "ZTJlLXByb2ZpbGUtc2VydmljZQ==.1") - .endpointName("/profile/{name}") + .serviceId("ZTJlLXByb2ZpbGUtc2VydmljZQ==.1") + .endpointName("{POST}/profile/{name}") .duration(1) .startTime(-1) .minDurationThreshold(1500) diff --git a/test/e2e/e2e-test/src/test/java/org/apache/skywalking/e2e/profile/ProfileE2E.java b/test/e2e/e2e-test/src/test/java/org/apache/skywalking/e2e/profile/ProfileE2E.java index b89b8d208..62f19a3c9 100644 --- a/test/e2e/e2e-test/src/test/java/org/apache/skywalking/e2e/profile/ProfileE2E.java +++ b/test/e2e/e2e-test/src/test/java/org/apache/skywalking/e2e/profile/ProfileE2E.java @@ -141,7 +141,7 @@ public class ProfileE2E extends SkyWalkingTestAdapter { void createProfileTask() throws Exception { final ProfileTaskCreationRequest creationRequest = ProfileTaskCreationRequest.builder() .serviceId("ZTJlLXByb2ZpbGUtc2VydmljZQ==.1") - .endpointName("/profile/{name}") + .endpointName("{POST}/profile/{name}") .duration(1) .startTime(-1) .minDurationThreshold(1500) diff --git a/test/e2e/e2e-test/src/test/resources/expected/profile/endpoints.yml b/test/e2e/e2e-test/src/test/resources/expected/profile/endpoints.yml index c9a192dd6..bcd8164fd 100644 --- a/test/e2e/e2e-test/src/test/resources/expected/profile/endpoints.yml +++ b/test/e2e/e2e-test/src/test/resources/expected/profile/endpoints.yml @@ -14,5 +14,5 @@ # limitations under the License. endpoints: - - key: ZTJlLXByb2ZpbGUtc2VydmljZQ==.1_L3Byb2ZpbGUve25hbWV9 - label: /profile/{name} + - key: ZTJlLXByb2ZpbGUtc2VydmljZQ==.1_e1BPU1R9L3Byb2ZpbGUve25hbWV9 + label: '{POST}/profile/{name}' diff --git a/test/e2e/e2e-test/src/test/resources/expected/profile/finished.yml b/test/e2e/e2e-test/src/test/resources/expected/profile/finished.yml index 6cf91baa6..e8bcf449e 100644 --- a/test/e2e/e2e-test/src/test/resources/expected/profile/finished.yml +++ b/test/e2e/e2e-test/src/test/resources/expected/profile/finished.yml @@ -16,7 +16,7 @@ tasks: - id: not null serviceId: ZTJlLXByb2ZpbGUtc2VydmljZQ==.1 - endpointName: /profile/{name} + endpointName: '{POST}/profile/{name}' startTime: gt 0 duration: gt 0 minDurationThreshold: gt 0 diff --git a/test/e2e/e2e-test/src/test/resources/expected/profile/notified.yml b/test/e2e/e2e-test/src/test/resources/expected/profile/notified.yml index 2d36360cb..03920eaca 100644 --- a/test/e2e/e2e-test/src/test/resources/expected/profile/notified.yml +++ b/test/e2e/e2e-test/src/test/resources/expected/profile/notified.yml @@ -16,7 +16,7 @@ tasks: - id: not null serviceId: ZTJlLXByb2ZpbGUtc2VydmljZQ==.1 - endpointName: /profile/{name} + endpointName: '{POST}/profile/{name}' startTime: gt 0 duration: gt 0 minDurationThreshold: gt 0 diff --git a/test/e2e/e2e-test/src/test/resources/expected/profile/profileSegment.yml b/test/e2e/e2e-test/src/test/resources/expected/profile/profileSegment.yml index 444c49f9b..672f07154 100644 --- a/test/e2e/e2e-test/src/test/resources/expected/profile/profileSegment.yml +++ b/test/e2e/e2e-test/src/test/resources/expected/profile/profileSegment.yml @@ -19,7 +19,7 @@ spans: serviceCode: not null startTime: gt 0 endTime: gt 0 - endpointName: /profile/{name} + endpointName: '{POST}/profile/{name}' tags: - key: url value: not null diff --git a/test/e2e/e2e-test/src/test/resources/expected/profile/profileSegments.yml b/test/e2e/e2e-test/src/test/resources/expected/profile/profileSegments.yml index 900758b81..0f9400dfa 100644 --- a/test/e2e/e2e-test/src/test/resources/expected/profile/profileSegments.yml +++ b/test/e2e/e2e-test/src/test/resources/expected/profile/profileSegments.yml @@ -16,7 +16,7 @@ traces: - key: not null endpointNames: - - /profile/{name} + - '{POST}/profile/{name}' duration: ge 0 start: gt 0 isError: false diff --git a/test/plugin/scenarios/elasticsearch-5.x-scenario/config/expectedData.yaml b/test/plugin/scenarios/elasticsearch-5.x-scenario/config/expectedData.yaml index 9c5e31ba1..5e05bd411 100644 --- a/test/plugin/scenarios/elasticsearch-5.x-scenario/config/expectedData.yaml +++ b/test/plugin/scenarios/elasticsearch-5.x-scenario/config/expectedData.yaml @@ -139,7 +139,7 @@ segmentItems: - {key: es.indices, value: ''} - {key: es.types, value: ''} skipAnalysis: 'false' - - operationName: /case/elasticsearch + - operationName: '{GET}/case/elasticsearch' operationId: 0 parentSpanId: -1 spanId: 0 diff --git a/test/plugin/scenarios/jdk-threading-scenario/config/expectedData.yaml b/test/plugin/scenarios/jdk-threading-scenario/config/expectedData.yaml index 4f05c0479..57ef73f6a 100644 --- a/test/plugin/scenarios/jdk-threading-scenario/config/expectedData.yaml +++ b/test/plugin/scenarios/jdk-threading-scenario/config/expectedData.yaml @@ -19,7 +19,7 @@ segmentItems: segments: - segmentId: not null spans: - - operationName: /greet/{username} + - operationName: '{GET}/greet/{username}' operationId: 0 parentSpanId: -1 spanId: 0 @@ -63,7 +63,7 @@ segmentItems: spanType: Local peer: '' refs: - - {parentEndpoint: '/greet/{username}', networkAddress: '', refType: CrossThread, + - {parentEndpoint: '{GET}/greet/{username}', networkAddress: '', refType: CrossThread, parentSpanId: 0, parentTraceSegmentId: not null, parentServiceInstance: not null, parentService: jdk-threading-scenario, traceId: not null} skipAnalysis: 'false' @@ -96,7 +96,7 @@ segmentItems: spanType: Local peer: '' refs: - - {parentEndpoint: '/greet/{username}', networkAddress: '', refType: CrossThread, + - {parentEndpoint: '{GET}/greet/{username}', networkAddress: '', refType: CrossThread, parentSpanId: 0, parentTraceSegmentId: not null, parentServiceInstance: not null, parentService: jdk-threading-scenario, traceId: not null} skipAnalysis: 'false' diff --git a/test/plugin/scenarios/spring-4.3.x-scenario/config/expectedData.yaml b/test/plugin/scenarios/spring-4.3.x-scenario/config/expectedData.yaml index 4bc9bfc60..474d9cf81 100644 --- a/test/plugin/scenarios/spring-4.3.x-scenario/config/expectedData.yaml +++ b/test/plugin/scenarios/spring-4.3.x-scenario/config/expectedData.yaml @@ -70,7 +70,7 @@ segmentItems: skipAnalysis: 'false' - segmentId: not null spans: - - operationName: /create/ + - operationName: '{POST}/create/' operationId: 0 parentSpanId: -1 spanId: 0 @@ -91,7 +91,7 @@ segmentItems: skipAnalysis: 'false' - segmentId: not null spans: - - operationName: /get/{id} + - operationName: '{GET}/get/{id}' operationId: 0 parentSpanId: -1 spanId: 0 @@ -112,7 +112,7 @@ segmentItems: skipAnalysis: 'false' - segmentId: not null spans: - - operationName: /update/{id} + - operationName: '{PUT}/update/{id}' operationId: 0 parentSpanId: -1 spanId: 0 @@ -133,7 +133,7 @@ segmentItems: skipAnalysis: 'false' - segmentId: not null spans: - - operationName: /delete/{id} + - operationName: '{DELETE}/delete/{id}' operationId: 0 parentSpanId: -1 spanId: 0 @@ -196,7 +196,7 @@ segmentItems: skipAnalysis: 'false' - segmentId: not null spans: - - operationName: /impl/restmapping + - operationName: '{GET}/impl/restmapping' operationId: 0 parentSpanId: -1 spanId: 0 diff --git a/test/plugin/scenarios/springmvc-reactive-scenario/config/expectedData.yaml b/test/plugin/scenarios/springmvc-reactive-scenario/config/expectedData.yaml index 3659a7696..0c18fbd63 100644 --- a/test/plugin/scenarios/springmvc-reactive-scenario/config/expectedData.yaml +++ b/test/plugin/scenarios/springmvc-reactive-scenario/config/expectedData.yaml @@ -36,7 +36,7 @@ segmentItems: - {key: db.instance, value: test} - {key: db.statement, value: not null} skipAnalysis: 'false' - - operationName: /testcase/{test} + - operationName: '{GET}/testcase/{test}' operationId: 0 parentSpanId: -1 spanId: 0