-
Notifications
You must be signed in to change notification settings - Fork 873
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
feat(analysis): add Apache SkyWalking as metrics provider #2491
Conversation
d441778
to
b160d8d
Compare
Codecov ReportBase: 81.60% // Head: 81.52% // Decreases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## master #2491 +/- ##
==========================================
- Coverage 81.60% 81.52% -0.09%
==========================================
Files 130 131 +1
Lines 19483 19562 +79
==========================================
+ Hits 15900 15947 +47
- Misses 2766 2795 +29
- Partials 817 820 +3
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
072b875
to
291f89d
Compare
Tests seem to be flaky, it can pass in my local machine |
Signed-off-by: kezhenxu94 <[email protected]>
Kudos, SonarCloud Quality Gate passed! 0 Bugs No Coverage information |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
I know we want to move metrics providers out of rollouts but until we're ready to do that, I think we should be very permissive on these kinds of merges.
#2514 solves supporting additional metrics providers without adding them to rollout source. @zachaller can confirm if we need this PR? |
Even though plugin system has just been merged it also is not 100% complete yet though it is close. I still do plan and think that merging this is the correct thing to do for a few reasons. One being that a good majority of users of this metric provider will be APISIX traffic router users the two projects are both under the apache foundation and share the same user base. We have already merged APISIX before any plugin systems where in place and this PR was also submitted way before any plugin systems where in place as well. Therefore I do think it makes sense to allow this PR through however this will probably be the last one that goes straight into the main project. I do think though that even with plugins having some form of graduation type system in place where plugins can gain enough traction and or meet some requirements that they move into "core" project however all that has not really been decided yet. |
Signed-off-by: zachaller <[email protected]>
Kudos, SonarCloud Quality Gate passed! 0 Bugs No Coverage information |
@zachaller thank you for helping to resolve the conflicts and merging the pr! |
Checklist:
"fix(controller): Updates such and such. Fixes #1234"
.