Skip to content
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

Include request method type in rest mapping annotations #5085

Merged
merged 31 commits into from Sep 5, 2020
Merged

Include request method type in rest mapping annotations #5085

merged 31 commits into from Sep 5, 2020

Conversation

BFergerson
Copy link
Member

@BFergerson BFergerson commented Jul 11, 2020


New feature or improvement

Using the @RequestMapping will result in an endpoint being saved like {GET}/users instead of /users. However, using the @GetMapping will result in /users. This PR will also result in the REST mapping annotations to work like the @RequestMapping annotation.

Also, spelling fix.

@codecov
Copy link

codecov bot commented Jul 11, 2020

Codecov Report

Merging #5085 into master will increase coverage by 0.44%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #5085      +/-   ##
============================================
+ Coverage     52.14%   52.59%   +0.44%     
- Complexity     3334     3366      +32     
============================================
  Files           893      893              
  Lines         22048    22048              
  Branches       2110     2110              
============================================
+ Hits          11498    11596      +98     
+ Misses         9628     9529      -99     
- Partials        922      923       +1     
Impacted Files Coverage Δ Complexity Δ
...er/core/server/auth/AuthenticationInterceptor.java 0.00% <0.00%> (-61.54%) 0.00% <0.00%> (-3.00%)
...er/sharing/server/ReceiverGRPCHandlerRegister.java 30.76% <0.00%> (-46.16%) 2.00% <0.00%> (-3.00%)
...ap/server/core/server/GRPCHandlerRegisterImpl.java 55.55% <0.00%> (-22.23%) 2.00% <0.00%> (-1.00%)
...r/cluster/plugin/standalone/StandaloneManager.java 77.77% <0.00%> (-22.23%) 3.00% <0.00%> (-1.00%)
...ing/oap/server/library/server/grpc/GRPCServer.java 53.22% <0.00%> (-4.84%) 6.00% <0.00%> (-1.00%)
...rary/client/elasticsearch/ElasticSearchClient.java 55.36% <0.00%> (-4.30%) 27.00% <0.00%> (-1.00%)
...er/sharing/server/SharingServerModuleProvider.java 40.00% <0.00%> (-2.67%) 8.00% <0.00%> (ø%)
...m/agent/core/remote/TraceSegmentServiceClient.java 68.49% <0.00%> (+1.36%) 15.00% <0.00%> (+1.00%)
.../server/core/alarm/provider/grpc/GRPCCallback.java 10.16% <0.00%> (+1.69%) 3.00% <0.00%> (+1.00%)
...apm/agent/core/remote/ServiceManagementClient.java 75.47% <0.00%> (+1.88%) 10.00% <0.00%> (ø%)
... and 22 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4f1dab7...296cf26. Read the comment docs.

@BFergerson
Copy link
Member Author

BFergerson commented Jul 11, 2020

@wu-sheng, what do you think about using {GET} in the endpoint names? I've noticed that makes some endpoint names like {GET}/users/{id}. It seems a bit odd like it indicates the {GET} is a dynamic property. I was thinking of maybe just changing it to [GET] so it would be [GET]/users/{id}.

What do you think? I noticed you have an issue about parsing URLs at (#329). Maybe you have in mind another solution? Like ignoring the endpoint name whenever the span's Tags.URL and Tags.HTTP.METHOD are set and just doing that combination on the backend? If it was done that way there would be automatic support for any plugin which has REST endpoints, etc.

@wu-sheng
Copy link
Member

Basically, adding http method in the operation name is not a big issue. But it mostly relates to rest style. In some systems, get and post are same, this naming style could be an issue by separating the same access.

From my understanding, we could provide an across plugin configuration, and makes different pluings share it.

Back to [] {}, it should not a big issue. Either is fine. We just need a consistent way.

@BFergerson BFergerson requested a review from wu-sheng July 15, 2020 16:20
Copy link
Member

@wu-sheng wu-sheng left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems you missed the adjustments of UT :) Please fix.

@wu-sheng wu-sheng added this to the 8.1.0 milestone Jul 16, 2020
@wu-sheng wu-sheng added agent Language agent related. enhancement Enhancement on performance or codes plugin Plugin for agent or collector. Be used to extend the capabilities of default implementor. labels Jul 16, 2020
Copy link
Member

@wu-sheng wu-sheng left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Look like the endpoint name changing affect the profiling e2e tests. Could you recheck?

@wu-sheng
Copy link
Member

Could you update this PR currently? 8.1.0 release is closing.

@BFergerson
Copy link
Member Author

@wu-sheng, I'm having trouble figuring out which e2e tests are failing and how I can run these tests locally. I can see how to write e2e tests (https://github.com/apache/skywalking/blob/master/docs/en/guides/README.md#writing-e2e-cases), but not how to run them. Is there a ./mvnw command I'm not seeing?

@wu-sheng
Copy link
Member

reas the file in the .github folder, there are a lot of yaml file controlling e2e. It is easy to follow. Those are GitHub action control files.

…ns/src/main/java/org/apache/skywalking/apm/plugin/spring/mvc/commons/interceptor/RestMappingMethodInterceptor.java
@wu-sheng
Copy link
Member

Seems Spring case is still failure. :)

@wu-sheng
Copy link
Member

@BFergerson Could you make the tests passed? This PR has been open for a little long time.

@wu-sheng
Copy link
Member

I think this has affected the profiling e2e case. I think it did profiling after matching the endpoint name.

@wu-sheng
Copy link
Member

@BFergerson
Copy link
Member Author

@wu-sheng, I was waiting for the e2e test to fail. Are you saying it's stuck in a loop? I'll try running the e2e tests locally in the meantime.

@BFergerson
Copy link
Member Author

BFergerson commented Aug 17, 2020

I'm not able to successfully run e2e tests locally.

I'm running:

git clone --recurse-submodules https://github.com/apache/skywalking.git
cd skywalking/
export SW_STORAGE=h2 && export SW_AGENT_JDK_VERSION=8 && export SKIP_TEST=true
make docker && ES_VERSION=es7 TAG=latest-es7 make docker.oap
cp -R dist test/e2e/
./mvnw --batch-mode -f test/e2e/pom.xml -am -DfailIfNoTests=false verify -Dit.test=org.apache.skywalking.e2e.profile.ProfileE2E

And I'm getting:

00:54:46.641 INFO  o.a.s.e2e.SkyWalkingAnnotations.<clinit> (108) - IDENTIFIER=skywalking-e2e-
00:54:46.644 INFO  o.a.s.e2e.SkyWalkingAnnotations.<clinit> (109) - LOG_DIR=/tmp/skywalking/logs
00:54:46.736 INFO  o.t.d.DockerClientProviderStrategy.lambda$getFirstValidStrategy$1 (110) - Loaded org.testcontainers.dockerclient.EnvironmentAndSystemPropertyClientProviderStrategy from ~/.testcontainers.properties, will try it first
00:54:47.051 INFO  o.t.d.EnvironmentAndSystemPropertyClientProviderStrategy.test (47) - Found docker client settings from environment
00:54:47.051 INFO  o.t.d.DockerClientProviderStrategy.lambda$getFirstValidStrategy$2 (119) - Found Docker environment with Environment variables, system properties and defaults. Resolved dockerHost=unix:///var/run/docker.sock
00:54:47.169 INFO  o.testcontainers.DockerClientFactory.client (137) - Docker host IP address is localhost
00:54:47.189 INFO  o.testcontainers.DockerClientFactory.client (144) - Connected to docker: 
  Server Version: 19.03.8
  API Version: 1.40
  Operating System: Ubuntu 20.04.1 LTS
  Total Memory: 31742 MB
00:54:48.378 WARN  o.t.utility.RegistryAuthLocator.lookupAuthConfig (135) - Failure when attempting to lookup auth config (dockerImageName: quay.io/testcontainers/ryuk:0.2.3, configFile: /home/brandon/.docker/config.json. Falling back to docker-java default behaviour. Exception message: /home/brandon/.docker/config.json (No such file or directory)
00:54:48.875 INFO  o.testcontainers.DockerClientFactory.client (156) - Ryuk started - will monitor and terminate Testcontainers containers on JVM exit
00:54:48.875 INFO  o.testcontainers.DockerClientFactory.client (167) - Checking the system...
00:54:48.876 INFO  o.testcontainers.DockerClientFactory.check (224) - ✔︎ Docker server version should be at least 1.6.0
00:54:48.973 INFO  o.testcontainers.DockerClientFactory.check (224) - ✔︎ Docker environment should have more than 2GB free disk space
00:54:49.443 INFO  o.a.s.e2e.SkyWalkingAnnotations.lambda$initDockerComposeField$2 (210) - /home/brandon/Desktop/test/skywalking/test/e2e/e2e-test/docker/profile/docker-compose.yml
[ERROR] Tests run: 1, Failures: 0, Errors: 1, Skipped: 0, Time elapsed: 3.067 s <<< FAILURE! - in org.apache.skywalking.e2e.profile.ProfileE2E
[ERROR] org.apache.skywalking.e2e.profile.ProfileE2E  Time elapsed: 3.066 s  <<< ERROR!
java.lang.NullPointerException

[INFO] 
[INFO] Results:
[INFO] 
[ERROR] Errors: 
[ERROR]   ProfileE2E » NullPointer
[INFO] 
[ERROR] Tests run: 1, Failures: 0, Errors: 1, Skipped: 0
[INFO] 
[INFO] 
[INFO] --- maven-failsafe-plugin:2.22.0:verify (default) @ e2e-test ---
[INFO] ------------------------------------------------------------------------
[INFO] Reactor Summary for SkyWalking End to End Tests 1.0.0:
[INFO] 
[INFO] SkyWalking End to End Tests ........................ SUCCESS [  1.360 s]
[INFO] e2e-common ......................................... SUCCESS [ 11.244 s]
[INFO] e2e-base ........................................... SUCCESS [  3.856 s]
[INFO] e2e-protocol ....................................... SUCCESS [  2.986 s]
[INFO] e2e-service-provider ............................... SUCCESS [  0.948 s]
[INFO] e2e-service-consumer ............................... SUCCESS [  0.541 s]
[INFO] e2e-test ........................................... FAILURE [  6.124 s]
[INFO] ------------------------------------------------------------------------
[INFO] BUILD FAILURE
[INFO] ------------------------------------------------------------------------
[INFO] Total time:  27.377 s
[INFO] Finished at: 2020-08-18T00:54:50+04:00
[INFO] ------------------------------------------------------------------------

When I run ProfileE2E directly I get:

java.lang.NullPointerException
	at org.apache.skywalking.e2e.SkyWalkingAnnotations.lambda$initDockerComposeField$2(SkyWalkingAnnotations.java:211)
	at java.base/java.util.ArrayList.forEach(ArrayList.java:1510)
	at org.apache.skywalking.e2e.SkyWalkingAnnotations.initDockerComposeField(SkyWalkingAnnotations.java:206)
	at org.apache.skywalking.e2e.SkyWalkingAnnotations.init(SkyWalkingAnnotations.java:115)
	at org.apache.skywalking.e2e.SkyWalkingExtension.beforeAll(SkyWalkingExtension.java:53)
	at org.junit.jupiter.engine.descriptor.ClassBasedTestDescriptor.lambda$invokeBeforeAllCallbacks$7(ClassBasedTestDescriptor.java:355)
	at org.junit.platform.engine.support.hierarchical.ThrowableCollector.execute(ThrowableCollector.java:73)
	at org.junit.jupiter.engine.descriptor.ClassBasedTestDescriptor.invokeBeforeAllCallbacks(ClassBasedTestDescriptor.java:355)
	at org.junit.jupiter.engine.descriptor.ClassBasedTestDescriptor.before(ClassBasedTestDescriptor.java:189)
	at org.junit.jupiter.engine.descriptor.ClassBasedTestDescriptor.before(ClassBasedTestDescriptor.java:77)
	at org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$5(NodeTestTask.java:132)
	at org.junit.platform.engine.support.hierarchical.ThrowableCollector.execute(ThrowableCollector.java:73)
	at org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$7(NodeTestTask.java:125)
	at org.junit.platform.engine.support.hierarchical.Node.around(Node.java:135)
	at org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$8(NodeTestTask.java:123)
	at org.junit.platform.engine.support.hierarchical.ThrowableCollector.execute(ThrowableCollector.java:73)
	at org.junit.platform.engine.support.hierarchical.NodeTestTask.executeRecursively(NodeTestTask.java:122)
	at org.junit.platform.engine.support.hierarchical.NodeTestTask.execute(NodeTestTask.java:80)
	at java.base/java.util.ArrayList.forEach(ArrayList.java:1510)
	at org.junit.platform.engine.support.hierarchical.SameThreadHierarchicalTestExecutorService.invokeAll(SameThreadHierarchicalTestExecutorService.java:38)
	at org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$5(NodeTestTask.java:139)
	at org.junit.platform.engine.support.hierarchical.ThrowableCollector.execute(ThrowableCollector.java:73)
	at org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$7(NodeTestTask.java:125)
	at org.junit.platform.engine.support.hierarchical.Node.around(Node.java:135)
	at org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$8(NodeTestTask.java:123)
	at org.junit.platform.engine.support.hierarchical.ThrowableCollector.execute(ThrowableCollector.java:73)
	at org.junit.platform.engine.support.hierarchical.NodeTestTask.executeRecursively(NodeTestTask.java:122)
	at org.junit.platform.engine.support.hierarchical.NodeTestTask.execute(NodeTestTask.java:80)
	at org.junit.platform.engine.support.hierarchical.SameThreadHierarchicalTestExecutorService.submit(SameThreadHierarchicalTestExecutorService.java:32)
	at org.junit.platform.engine.support.hierarchical.HierarchicalTestExecutor.execute(HierarchicalTestExecutor.java:57)
	at org.junit.platform.engine.support.hierarchical.HierarchicalTestEngine.execute(HierarchicalTestEngine.java:51)
	at org.junit.platform.launcher.core.DefaultLauncher.execute(DefaultLauncher.java:229)
	at org.junit.platform.launcher.core.DefaultLauncher.lambda$execute$6(DefaultLauncher.java:197)
	at org.junit.platform.launcher.core.DefaultLauncher.withInterceptedStreams(DefaultLauncher.java:211)
	at org.junit.platform.launcher.core.DefaultLauncher.execute(DefaultLauncher.java:191)
	at org.junit.platform.launcher.core.DefaultLauncher.execute(DefaultLauncher.java:128)
	at com.intellij.junit5.JUnit5IdeaTestRunner.startRunnerWithArgs(JUnit5IdeaTestRunner.java:71)
	at com.intellij.rt.junit.IdeaTestRunner$Repeater.startRunnerWithArgs(IdeaTestRunner.java:33)
	at com.intellij.rt.junit.JUnitStarter.prepareStreamsAndStart(JUnitStarter.java:220)
	at com.intellij.rt.junit.JUnitStarter.main(JUnitStarter.java:53)
	Suppressed: java.lang.NullPointerException
		at org.apache.skywalking.e2e.SkyWalkingAnnotations.lambda$destroy$0(SkyWalkingAnnotations.java:137)
		at java.base/java.util.Optional.ifPresent(Optional.java:176)
		at org.apache.skywalking.e2e.SkyWalkingAnnotations.destroy(SkyWalkingAnnotations.java:134)
		at org.apache.skywalking.e2e.SkyWalkingExtension.afterAll(SkyWalkingExtension.java:58)
		at org.junit.jupiter.engine.descriptor.ClassBasedTestDescriptor.lambda$invokeAfterAllCallbacks$13(ClassBasedTestDescriptor.java:421)
		at org.junit.platform.engine.support.hierarchical.ThrowableCollector.execute(ThrowableCollector.java:73)
		at org.junit.jupiter.engine.descriptor.ClassBasedTestDescriptor.lambda$invokeAfterAllCallbacks$14(ClassBasedTestDescriptor.java:421)
		at java.base/java.util.ArrayList.forEach(ArrayList.java:1510)
		at org.junit.jupiter.engine.descriptor.ClassBasedTestDescriptor.invokeAfterAllCallbacks(ClassBasedTestDescriptor.java:421)
		at org.junit.jupiter.engine.descriptor.ClassBasedTestDescriptor.after(ClassBasedTestDescriptor.java:213)
		at org.junit.jupiter.engine.descriptor.ClassBasedTestDescriptor.after(ClassBasedTestDescriptor.java:77)
		at org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$6(NodeTestTask.java:145)
		at org.junit.platform.engine.support.hierarchical.ThrowableCollector.execute(ThrowableCollector.java:73)
		at org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$7(NodeTestTask.java:145)
		... 27 more

Which I think is the same NPE as when I run via ./mvnw.

@wu-sheng
Copy link
Member

@kezhenxu94 Could you take a look the error?

@BFergerson I think the key error is here, https://github.com/apache/skywalking/blob/master/test/e2e/e2e-test/src/test/java/org/apache/skywalking/e2e/profile/ProfileE2E.java#L144
You changed the endpoint naming rule, then the profile task fails because no endpoint name matched the task parameter.

Note, the logs you posted are not related to this, I just say for the e2e test fail because of timeout.

@wu-sheng
Copy link
Member

wu-sheng commented Sep 4, 2020

@BFergerson Any update of this PR?

@BFergerson
Copy link
Member Author

BFergerson commented Sep 4, 2020

@wu-sheng, I'm willing to fix this if I can run these tests necessary to confirm it works. However, I'm not even able to get the correct e2e tests to run successfully. These are the commands I'm using:

git clone --recurse-submodules https://github.com/apache/skywalking.git
cd skywalking/
export SW_STORAGE=h2 && export SW_AGENT_JDK_VERSION=8 && export SKIP_TEST=true
make docker && ES_VERSION=es7 TAG=latest-es7 make docker.oap
cp -R dist test/e2e/
./mvnw --batch-mode -f test/e2e/pom.xml -am -DfailIfNoTests=false verify -Dit.test=org.apache.skywalking.e2e.profile.ProfileE2E

The issue I get is the same as the one I stated above. Are you able to run those commands successfully? What am I doing wrong?


Local Info:

$ uname -a
Linux brandon-XPS-15-7590 5.4.0-45-generic #49-Ubuntu SMP Wed Aug 26 13:38:52 UTC 2020 x86_64 x86_64 x86_64 GNU/Linux

$ docker -v
Docker version 19.03.8, build afacb8b7f0

$ java -version
openjdk version "1.8.0_265"
OpenJDK Runtime Environment (build 1.8.0_265-8u265-b01-0ubuntu2~20.04-b01)
OpenJDK 64-Bit Server VM (build 25.265-b01, mixed mode)

@kezhenxu94
Copy link
Member

@BFergerson my local environment is unavailable now because of the upgrade of Docker version, before I can make it work again, can you just add a breakpoint in SkyWalkingAnnotations.java:211 and evaluate dockerComposeFile to see where the NPE came from

@kezhenxu94
Copy link
Member

@BFergerson there is a bug in the E2E test when running locally, please export GITHUB_RUN_ID=whatever as well, and I'll ask the student who wrote this part to fix it soon

@BFergerson
Copy link
Member Author

@kezhenxu94, thanks. I was able to run e2e tests after adding that environment variable. This PR should be good to go now.

Copy link
Member

@wu-sheng wu-sheng left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Cool! @BFergerson from your experiences, seems there are some documents missing.

Could you try to fix and enhance this doc, https://github.com/apache/skywalking/blob/master/docs/en/guides/README.md#end-to-end-tests-e2e-for-short

@wu-sheng wu-sheng merged commit f8c887e into apache:master Sep 5, 2020
vcjmhg pushed a commit to vcjmhg/skywalking that referenced this pull request Sep 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
agent Language agent related. enhancement Enhancement on performance or codes plugin Plugin for agent or collector. Be used to extend the capabilities of default implementor.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants