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.

Sign up for GitHub

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

Jump to bottom

Intercept feign URL without parameters #3854

Merged
merged 21 commits into from Nov 17, 2019
Merged

Intercept feign URL without parameters #3854

merged 21 commits into from Nov 17, 2019

Conversation

dominicqi
Copy link
Contributor

…out parameters

Please answer these questions before submitting pull request

Bug fix

  • Bug description.

  • How to fix?

New feature or improvement

  • Describe the details and related test reports.
    intercept feign.ReflectiveFeign.BuildTemplateByResolvingArgs#resolve(Object[], RequestTemplate, Map) get url without params before resolve ,put it into ThreadLocal ,get the url in Feign DefaultHttpClientInterceptor

@dominicqi
Copy link
Contributor Author

image
Here is the screenshots

@wu-sheng
Copy link
Member

Let's wait for #3838 merged, then, you need to add this case into the test.

@wu-sheng wu-sheng added this to the 6.6.0 milestone Nov 14, 2019
@wu-sheng
Copy link
Member

FYI @Aderm

@arugal
Copy link
Member

arugal commented Nov 14, 2019

CI test failed, please check.

@wu-sheng wu-sheng added agent Language agent related. feature New feature plugin Plugin for agent or collector. Be used to extend the capabilities of default implementor. labels Nov 14, 2019
@wu-sheng
Copy link
Member

You still get the code style issue. You should check locally first.

@arugal
Copy link
Member

arugal commented Nov 14, 2019

/run agent-plugin-test-3

@wu-sheng
Copy link
Member

Feign test #3838 has been merged. Please add this into the test.

@wu-sheng
Copy link
Member

You could find how to run and test through this doc

https://github.com/apache/skywalking/blob/master/docs/en/guides/Plugin-test.md

@wu-sheng
Copy link
Member

@dominicqi Please make sure the feign-scenario passed, and include your new feature in.

@dominicqi
Copy link
Contributor Author

i found the problem, RequestTemplate#url miss the url part in the http://localhost:8080/feign-scenario,i will try to fix this

fix: miss path in feign target url like "http://localhost:8080/feign-…
f7bac66 
…scenario"

     fix feign-scenario test use pathVariable
@dominicqi
Copy link
Contributor Author

/run agent-plugin-test-3

wu-sheng
wu-sheng previously approved these changes Nov 16, 2019
View reviewed changes
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

arugal
arugal reviewed Nov 16, 2019
View reviewed changes
...c/main/java/org/apache/skywalking/apm/plugin/feign/http/v9/DefaultHttpClientInterceptor.java
if (feignResolvedURL != null) {
PathVarInterceptor.URL_CONTEXT.remove();
}
}
if (operationName == null || operationName.length() == 0) {
operationName = "/";
Copy link
Member

Choose a reason for hiding this comment

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

If operationName could be null, operationName.replace would already be NPE

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the operationName never be null, because URL.getPath() will return empty str。 I will remove this null condition.

Copy link
Member

@arugal arugal Nov 17, 2019 •
edited

Choose a reason for hiding this comment

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

It seems better this way. right?

        FeignResolvedURL feignResolvedURL = PathVarInterceptor.URL_CONTEXT.get();
        if (feignResolvedURL != null) {
            try {
                 if (operationName.length() > 0) {
                     operationName = operationName.replace(feignResolvedURL.getUrl(), feignResolvedURL.getOriginUrl());
                  }
            } finally {
                PathVarInterceptor.URL_CONTEXT.remove();
            }
        }
        if (operationName.length() == 0) {
            operationName = "/";
        }

Copy link
Member

Choose a reason for hiding this comment

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

No matter different. Change or not seems OK to me.

Copy link
Member

Choose a reason for hiding this comment

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

No matter different.

ha ha, it seems so.

wu-sheng
wu-sheng reviewed Nov 16, 2019
View reviewed changes
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.

@dominicqi Please follow the review suggestions, they make sense to me.

arugal
arugal approved these changes Nov 17, 2019
View reviewed changes
Copy link
Member

@arugal arugal left a comment

Choose a reason for hiding this comment

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

LGTM

@wu-sheng
Copy link
Member

/run agent-plugin-test-2

@wu-sheng wu-sheng merged commit 58149a7 into apache:master Nov 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wu-sheng wu-sheng wu-sheng approved these changes

@arugal arugal arugal approved these changes

@dmsolr dmsolr Awaiting requested review from dmsolr

@ascrutae ascrutae Awaiting requested review from ascrutae

Assignees
No one assigned
Labels
agent Language agent related. feature New feature plugin Plugin for agent or collector. Be used to extend the capabilities of default implementor.
Projects
None yet
Milestone
6.6.0
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@dominicqi @wu-sheng @arugal