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

Feature: support endpoint name grouping by OpenAPI definitions. #7130

Merged
merged 21 commits into from
Jun 19, 2021

Conversation

wankai123
Copy link
Member

@wankai123 wankai123 commented Jun 18, 2021

Support endpoint name grouping by OpenAPI definitions

  • If this is non-trivial feature, paste the links/URLs to the design doc.

  • Update the documentation to include this new feature.

  • Tests(including UT, IT, E2E) are added to verify the new feature.

  • If it's UI related, attach the screenshots below.

  • If this pull request closes/resolves/fixes an existing issue, replace the issue number. Closes #.

  • Update the CHANGES log.

Related to #7113
More information about the feature please see docs/en/setup/backend/endpoint-grouping-rules.md

@codecov
Copy link

codecov bot commented Jun 18, 2021

Codecov Report

Merging #7130 (e014c05) into master (002f4c2) will increase coverage by 2.65%.
The diff coverage is 48.74%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #7130      +/-   ##
============================================
+ Coverage     50.35%   53.01%   +2.65%     
- Complexity     2692     4263    +1571     
============================================
  Files           740     1876    +1136     
  Lines         18265    40707   +22442     
  Branches       1766     4534    +2768     
============================================
+ Hits           9198    21579   +12381     
- Misses         8337    18063    +9726     
- Partials        730     1065     +335     
Impacted Files Coverage Δ
.../log4j/v1/x/SkyWalkingContextPatternConverter.java 0.00% <0.00%> (ø)
...oolkit/log/log4j/v1/x/TraceIdPatternConverter.java 0.00% <ø> (ø)
...m/toolkit/log/log4j/v1/x/TraceIdPatternLayout.java 0.00% <ø> (ø)
...m/toolkit/log/log4j/v1/x/TraceIdPatternParser.java 0.00% <0.00%> (ø)
...lkit/log/log4j/v1/x/log/GRPCLogClientAppender.java 0.00% <0.00%> (ø)
...m/toolkit/log/log4j/v2/x/Log4j2OutputAppender.java 0.00% <ø> (ø)
...4j/v2/x/Log4j2SkyWalkingContextOutputAppender.java 0.00% <0.00%> (ø)
...kit/log/log4j/v2/x/SkyWalkingContextConverter.java 0.00% <0.00%> (ø)
...lkit/log/log4j/v2/x/log/GRPCLogClientAppender.java 0.00% <0.00%> (ø)
...lkit/log/logback/v1/x/LogbackPatternConverter.java 0.00% <ø> (ø)
... and 1880 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 528ee6d...e014c05. Read the comment docs.

@wu-sheng wu-sheng added this to the 8.7.0 milestone Jun 18, 2021
@wu-sheng wu-sheng added backend OAP backend related. feature New feature labels Jun 18, 2021
wankai123 and others added 2 commits June 18, 2021 12:46
fix/polish some typo and codes

Co-authored-by: kezhenxu94 <[email protected]>
kezhenxu94
kezhenxu94 previously approved these changes Jun 18, 2021
Copy link
Member

@kezhenxu94 kezhenxu94 left a comment

Choose a reason for hiding this comment

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

LGTM once CI pass. Thanks!!!

@kezhenxu94 kezhenxu94 requested a review from wu-sheng June 18, 2021 10:37
wu-sheng
wu-sheng previously approved these changes Jun 19, 2021
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.

It is good for me. Thanks.

kezhenxu94
kezhenxu94 previously approved these changes Jun 19, 2021
Comment on lines 47 to 51
rule.addGroupedRule("serviceA", "GET:/products1/{id}/" + +i, "GET:/products1/([^/]+)/" + i);
rule.addGroupedRule("serviceA", "POST:/products1/{id}/" + +i, "POST:/products1/([^/]+)/" + i);
rule.addGroupedRule("serviceA", "GET:/products2/{id}/" + +i, "GET:/products2/([^/]+)/" + i);
rule.addGroupedRule("serviceA", "POST:/products3/{id}/" + +i, "POST:/products3/([^/]+)/" + i);
rule.addGroupedRule("serviceA", "GET:/products3/{id}/" + +i, "GET:/products3/([^/]+)/" + i);
Copy link
Member

@kezhenxu94 kezhenxu94 Jun 19, 2021

Choose a reason for hiding this comment

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

Not important though, "GET:/products1/{id}/" + +i looks tricky, why not just "GET:/products1/{id}/" + i?

Update:

I think what you want is "GET:/products1/{id}/" + ++i

Copy link
Member

Choose a reason for hiding this comment

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

Haha, interesting, it is better to fix before merge.

Copy link
Member Author

Choose a reason for hiding this comment

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

thanks, @kezhenxu94, I tested +i but forgot to remove them.

Comment on lines 68 to 72
rule.addGroupedRule("serviceA", "GET:/products1/{id}/" + +i, "GET:/products1/([^/]+)/" + i);
rule.addGroupedRule("serviceA", "POST:/products1/{id}/" + +i, "POST:/products1/([^/]+)/" + i);
rule.addGroupedRule("serviceA", "GET:/products2/{id}/" + +i, "GET:/products2/([^/]+)/" + i);
rule.addGroupedRule("serviceA", "POST:/products3/{id}/" + +i, "POST:/products3/([^/]+)/" + i);
rule.addGroupedRule("serviceA", "GET:/products3/{id}/" + +i, "GET:/products3/([^/]+)/" + i);
Copy link
Member

Choose a reason for hiding this comment

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

Same here

Comment on lines 89 to 93
rule.addGroupedRule("serviceA", "GET:/products1/{id}/" + +i, "GET:/products1/([^/]+)/" + i);
rule.addGroupedRule("serviceA", "POST:/products1/{id}/" + +i, "POST:/products1/([^/]+)/" + i);
rule.addGroupedRule("serviceA", "GET:/products2/{id}/" + +i, "GET:/products2/([^/]+)/" + i);
rule.addGroupedRule("serviceA", "POST:/products3/{id}/" + +i, "POST:/products3/([^/]+)/" + i);
rule.addGroupedRule("serviceA", "GET:/products3/{id}/" + +i, "GET:/products3/([^/]+)/" + i);
Copy link
Member

Choose a reason for hiding this comment

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

And here

@wankai123 wankai123 dismissed stale reviews from kezhenxu94 and wu-sheng via e014c05 June 19, 2021 16:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend OAP backend related. feature New feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants