You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@skywalking.apache.org by GitBox <gi...@apache.org> on 2020/02/14 15:21:29 UTC
[GitHub] [skywalking] kezhenxu94 opened a new pull request #4362: Adapt
Armeria plugin to version 0.98.0
kezhenxu94 opened a new pull request #4362: Adapt Armeria plugin to version 0.98.0
URL: https://github.com/apache/skywalking/pull/4362
### Motivation:
Support Armeria 0.98.0 in its plugin.
### Modifications:
Add an intercept point that is refactored in Armeria 0.98.
### Result:
Now Armeria plugin supports 0.98.0, and 0.98.1
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [skywalking] wu-sheng commented on issue #4362: Adapt Armeria
plugin to version 0.98.0
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on issue #4362: Adapt Armeria plugin to version 0.98.0
URL: https://github.com/apache/skywalking/pull/4362#issuecomment-586542152
Tests seem passed, please continue.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [skywalking] wu-sheng commented on a change in pull request #4362:
Adapt Armeria plugin to version 0.98.0
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on a change in pull request #4362: Adapt Armeria plugin to version 0.98.0
URL: https://github.com/apache/skywalking/pull/4362#discussion_r379510143
##########
File path: apm-sniffer/apm-sdk-plugin/armeria-0.85.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/armeria/define/Armeria086ClientInstrumentation.java
##########
@@ -32,6 +32,7 @@
public class Armeria086ClientInstrumentation extends ClassInstanceMethodsEnhancePluginDefine {
Review comment:
> For example Armeria096ClientInstrumentation indicates that it supports 0.96+
That totally works for me.
> But I’m recently consider to rewrite the plugin from 1.0.0 with a more recommended tracing solution in Armeria, decorator, but that’s another story.
If that could replace some of the existing plugins, we could delete them later. It is not an issue.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [skywalking] kezhenxu94 commented on a change in pull request
#4362: Adapt Armeria plugin to version 0.98.0
Posted by GitBox <gi...@apache.org>.
kezhenxu94 commented on a change in pull request #4362: Adapt Armeria plugin to version 0.98.0
URL: https://github.com/apache/skywalking/pull/4362#discussion_r379497366
##########
File path: apm-sniffer/apm-sdk-plugin/armeria-0.85.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/armeria/define/Armeria086ClientInstrumentation.java
##########
@@ -32,6 +32,7 @@
public class Armeria086ClientInstrumentation extends ClassInstanceMethodsEnhancePluginDefine {
Review comment:
Sure, what about renaming to the lowest version that the instrumentation supports from? For example Armeria096ClientInstrumentation indicates that it supports 0.96+, and if it supports all future versions that would be best, if it doesn’t support from, for example 1.2.3, we will have another Armeria123...?
But I’m recently consider to rewrite the plugin from 1.0.0 with a more recommended tracing solution in Armeria, decorator, but that’s another story.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [skywalking] kezhenxu94 merged pull request #4362: Adapt Armeria
plugin to version 0.98.0
Posted by GitBox <gi...@apache.org>.
kezhenxu94 merged pull request #4362: Adapt Armeria plugin to version 0.98.0
URL: https://github.com/apache/skywalking/pull/4362
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [skywalking] wu-sheng commented on a change in pull request #4362:
Adapt Armeria plugin to version 0.98.0
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on a change in pull request #4362: Adapt Armeria plugin to version 0.98.0
URL: https://github.com/apache/skywalking/pull/4362#discussion_r379493454
##########
File path: test/plugin/scenarios/armeria-0.96plus-scenario/support-version.list
##########
@@ -14,5 +14,6 @@
# See the License for the specific language governing permissions and
# limitations under the License.
+0.98.0
Review comment:
Make sense to me.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [skywalking] kezhenxu94 commented on a change in pull request
#4362: Adapt Armeria plugin to version 0.98.0
Posted by GitBox <gi...@apache.org>.
kezhenxu94 commented on a change in pull request #4362: Adapt Armeria plugin to version 0.98.0
URL: https://github.com/apache/skywalking/pull/4362#discussion_r379488858
##########
File path: test/plugin/scenarios/armeria-0.96plus-scenario/support-version.list
##########
@@ -14,5 +14,6 @@
# See the License for the specific language governing permissions and
# limitations under the License.
+0.98.0
Review comment:
@wu-sheng let me add this version to verify it works (although I verified locally), and revert this to merge #4328 :)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [skywalking] kezhenxu94 commented on issue #4362: Adapt Armeria
plugin to version 0.98.0
Posted by GitBox <gi...@apache.org>.
kezhenxu94 commented on issue #4362: Adapt Armeria plugin to version 0.98.0
URL: https://github.com/apache/skywalking/pull/4362#issuecomment-586549323
The failure is due to network issue and I had re-run it several times, I'll merge this instead of re-running over and over
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [skywalking] kezhenxu94 commented on issue #4362: Adapt Armeria
plugin to version 0.98.0
Posted by GitBox <gi...@apache.org>.
kezhenxu94 commented on issue #4362: Adapt Armeria plugin to version 0.98.0
URL: https://github.com/apache/skywalking/pull/4362#issuecomment-586335019
here are the changes between 0.97.0 and 0.98.0 that breaks the plugin
https://github.com/line/armeria/blob/aba612a0e6b980334c5a40818047efe2aecc8e4e/core/src/main/java/com/linecorp/armeria/client/UserClient.java#L133-L135
https://github.com/line/armeria/blob/d5ba7e73cd6d4df72e6ed55fe9153b6f7a7f19e9/core/src/main/java/com/linecorp/armeria/client/UserClient.java#L145-L147
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [skywalking] kezhenxu94 edited a comment on issue #4362: Adapt
Armeria plugin to version 0.98.0
Posted by GitBox <gi...@apache.org>.
kezhenxu94 edited a comment on issue #4362: Adapt Armeria plugin to version 0.98.0
URL: https://github.com/apache/skywalking/pull/4362#issuecomment-586549323
The failure is due to network issue and I had re-run it several times, I'll merge this after the running tests finished, instead of re-running over and over
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [skywalking] wu-sheng commented on issue #4362: Adapt Armeria
plugin to version 0.98.0
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on issue #4362: Adapt Armeria plugin to version 0.98.0
URL: https://github.com/apache/skywalking/pull/4362#issuecomment-586549638
Go for that. It is fine.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [skywalking] codecov-io commented on issue #4362: Adapt Armeria
plugin to version 0.98.0
Posted by GitBox <gi...@apache.org>.
codecov-io commented on issue #4362: Adapt Armeria plugin to version 0.98.0
URL: https://github.com/apache/skywalking/pull/4362#issuecomment-586545936
# [Codecov](https://codecov.io/gh/apache/skywalking/pull/4362?src=pr&el=h1) Report
> Merging [#4362](https://codecov.io/gh/apache/skywalking/pull/4362?src=pr&el=desc) into [master](https://codecov.io/gh/apache/skywalking/commit/ae442e364f88f3bee35b327f32c07097af00c060?src=pr&el=desc) will **decrease** coverage by `0.01%`.
> The diff coverage is `0%`.
[](https://codecov.io/gh/apache/skywalking/pull/4362?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #4362 +/- ##
==========================================
- Coverage 26.16% 26.14% -0.02%
==========================================
Files 1181 1182 +1
Lines 26910 26922 +12
Branches 3713 3714 +1
==========================================
- Hits 7040 7039 -1
- Misses 19236 19248 +12
- Partials 634 635 +1
```
| [Impacted Files](https://codecov.io/gh/apache/skywalking/pull/4362?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [...pm/plugin/armeria/Armeria085ServerInterceptor.java](https://codecov.io/gh/apache/skywalking/pull/4362/diff?src=pr&el=tree#diff-YXBtLXNuaWZmZXIvYXBtLXNkay1wbHVnaW4vYXJtZXJpYS0wLjg1LngtcGx1Z2luL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9za3l3YWxraW5nL2FwbS9wbHVnaW4vYXJtZXJpYS9Bcm1lcmlhMDg1U2VydmVySW50ZXJjZXB0b3IuamF2YQ==) | `0% <ø> (ø)` | :arrow_up: |
| [...pm/plugin/armeria/Armeria085ClientInterceptor.java](https://codecov.io/gh/apache/skywalking/pull/4362/diff?src=pr&el=tree#diff-YXBtLXNuaWZmZXIvYXBtLXNkay1wbHVnaW4vYXJtZXJpYS0wLjg1LngtcGx1Z2luL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9za3l3YWxraW5nL2FwbS9wbHVnaW4vYXJtZXJpYS9Bcm1lcmlhMDg1Q2xpZW50SW50ZXJjZXB0b3IuamF2YQ==) | `0% <ø> (ø)` | :arrow_up: |
| [...pm/plugin/armeria/Armeria086ClientInterceptor.java](https://codecov.io/gh/apache/skywalking/pull/4362/diff?src=pr&el=tree#diff-YXBtLXNuaWZmZXIvYXBtLXNkay1wbHVnaW4vYXJtZXJpYS0wLjg1LngtcGx1Z2luL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9za3l3YWxraW5nL2FwbS9wbHVnaW4vYXJtZXJpYS9Bcm1lcmlhMDg2Q2xpZW50SW50ZXJjZXB0b3IuamF2YQ==) | `0% <ø> (ø)` | :arrow_up: |
| [...g/apm/plugin/armeria/ArmeriaClientInterceptor.java](https://codecov.io/gh/apache/skywalking/pull/4362/diff?src=pr&el=tree#diff-YXBtLXNuaWZmZXIvYXBtLXNkay1wbHVnaW4vYXJtZXJpYS0wLjg1LngtcGx1Z2luL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9za3l3YWxraW5nL2FwbS9wbHVnaW4vYXJtZXJpYS9Bcm1lcmlhQ2xpZW50SW50ZXJjZXB0b3IuamF2YQ==) | `0% <ø> (ø)` | :arrow_up: |
| [...pm/plugin/armeria/Armeria098ClientInterceptor.java](https://codecov.io/gh/apache/skywalking/pull/4362/diff?src=pr&el=tree#diff-YXBtLXNuaWZmZXIvYXBtLXNkay1wbHVnaW4vYXJtZXJpYS0wLjg1LngtcGx1Z2luL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9za3l3YWxraW5nL2FwbS9wbHVnaW4vYXJtZXJpYS9Bcm1lcmlhMDk4Q2xpZW50SW50ZXJjZXB0b3IuamF2YQ==) | `0% <0%> (ø)` | |
| [...ache/skywalking/apm/agent/core/jvm/JVMService.java](https://codecov.io/gh/apache/skywalking/pull/4362/diff?src=pr&el=tree#diff-YXBtLXNuaWZmZXIvYXBtLWFnZW50LWNvcmUvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3NreXdhbGtpbmcvYXBtL2FnZW50L2NvcmUvanZtL0pWTVNlcnZpY2UuamF2YQ==) | `58.33% <0%> (-1.67%)` | :arrow_down: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/skywalking/pull/4362?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/skywalking/pull/4362?src=pr&el=footer). Last update [ae442e3...e4196e5](https://codecov.io/gh/apache/skywalking/pull/4362?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [skywalking] wu-sheng commented on a change in pull request #4362:
Adapt Armeria plugin to version 0.98.0
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on a change in pull request #4362: Adapt Armeria plugin to version 0.98.0
URL: https://github.com/apache/skywalking/pull/4362#discussion_r379491221
##########
File path: apm-sniffer/apm-sdk-plugin/armeria-0.85.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/armeria/define/Armeria086ClientInstrumentation.java
##########
@@ -32,6 +32,7 @@
public class Armeria086ClientInstrumentation extends ClassInstanceMethodsEnhancePluginDefine {
Review comment:
This class name is not suitable anymore. Please consider to refactor?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services