You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@skywalking.apache.org by "zhangrm (via GitHub)" <gi...@apache.org> on 2023/02/19 03:49:38 UTC
[GitHub] [skywalking-java] zhangrm opened a new pull request, #459: Adapt Armeria's apm-sdk-plugin (client and server) version from 0.98x to the latest version 1.21. x
zhangrm opened a new pull request, #459:
URL: https://github.com/apache/skywalking-java/pull/459
<!--
β οΈ Please make sure to read this template first, pull requests that don't accord with this template
maybe closed without notice.
Texts surrounded by `<` and `>` are meant to be replaced by you, e.g. <framework name>, <issue number>.
Put an `x` in the `[ ]` to mark the item as CHECKED. `[x]`
-->
<!-- ==== π Remove this line WHEN AND ONLY WHEN you're fixing a bug, follow the checklist π ====
### Fix <bug description or the bug issue number or bug issue link>
- [ ] Add a unit test to verify that the fix works.
- [ ] Explain briefly why the bug exists and how to fix it.
==== π Remove this line WHEN AND ONLY WHEN you're fixing a bug, follow the checklist π ==== -->
<!-- ==== π Remove this line WHEN AND ONLY WHEN you're adding a new plugin, follow the checklist π ====
### Add an agent plugin to support <framework name>
- [ ] Add a test case for the new plugin, refer to [the doc](https://github.com/apache/skywalking-java/blob/main/docs/en/setup/service-agent/java-agent/Plugin-test.md)
- [ ] Add a component id in [the component-libraries.yml](https://github.com/apache/skywalking/blob/master/oap-server/server-starter/src/main/resources/component-libraries.yml)
- [ ] Add a logo in [the UI repo](https://github.com/apache/skywalking-rocketbot-ui/tree/master/src/views/components/topology/assets)
==== π Remove this line WHEN AND ONLY WHEN you're adding a new plugin, follow the checklist π ==== -->
<!-- ==== π Remove this line WHEN AND ONLY WHEN you're improving the performance, follow the checklist π ====
### Improve the performance of <class or module or ...>
- [ ] Add a benchmark for the improvement, refer to [the existing ones](https://github.com/apache/skywalking-java/blob/main/apm-commons/apm-datacarrier/src/test/java/org/apache/skywalking/apm/commons/datacarrier/LinkedArrayBenchmark.java)
- [ ] The benchmark result.
```text
<Paste the benchmark results here>
```
- [ ] Links/URLs to the theory proof or discussion articles/blogs. <links/URLs here>
==== π Remove this line WHEN AND ONLY WHEN you're improving the performance, follow the checklist π ==== -->
<!-- ==== π Remove this line WHEN AND ONLY WHEN you're adding a new feature, follow the checklist π ====
### <Feature description>
- [ ] 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.
==== π Remove this line WHEN AND ONLY WHEN you're adding a new feature, follow the checklist π ==== -->
- [ ] If this pull request closes/resolves/fixes an existing issue, replace the issue number. Closes #10278.
- [ ] Update the [`CHANGES` log](https://github.com/apache/skywalking-java/blob/main/CHANGES.md).
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] wu-sheng commented on pull request #459: Adapt Armeria's apm-sdk-plugin (client and server) version from 0.98x to the latest version 1.21. x
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on PR #459:
URL: https://github.com/apache/skywalking-java/pull/459#issuecomment-1436258989
Notice, you should move the other two Armeria plugins into the same place.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] wu-sheng merged pull request #459: Adapt Armeria's plugins to the latest version 1.22.x
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng merged PR #459:
URL: https://github.com/apache/skywalking-java/pull/459
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] wu-sheng commented on a diff in pull request #459: Adapt Armeria's apm-sdk-plugin (client and server) version from 0.98x to the latest version 1.22 x
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on code in PR #459:
URL: https://github.com/apache/skywalking-java/pull/459#discussion_r1112428144
##########
apm-sniffer/apm-sdk-plugin/armeria-0.85.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/armeria/Armeria085ServerInterceptor.java:
##########
@@ -32,13 +31,15 @@
import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.MethodInterceptResult;
import org.apache.skywalking.apm.network.trace.component.ComponentsDefine;
+import java.lang.reflect.Method;
+
@SuppressWarnings("unused") // actually used
public class Armeria085ServerInterceptor implements InstanceMethodsAroundInterceptor {
@Override
public void beforeMethod(final EnhancedInstance objInst, final Method method, final Object[] allArguments,
final Class<?>[] argumentsTypes, final MethodInterceptResult result) {
- DefaultHttpRequest httpRequest = (DefaultHttpRequest) allArguments[1];
+ HttpRequest httpRequest = (HttpRequest) allArguments[1];
Review Comment:
I think you don't need to change this. As this plugin should only work when version < 0.99. If not, it means you need another witness class in 0.85-0.99 plugin.
##########
apm-sniffer/apm-sdk-plugin/armeria-0.85.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/armeria/Armeria085ServerInterceptor.java:
##########
@@ -32,13 +31,15 @@
import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.MethodInterceptResult;
import org.apache.skywalking.apm.network.trace.component.ComponentsDefine;
+import java.lang.reflect.Method;
+
@SuppressWarnings("unused") // actually used
public class Armeria085ServerInterceptor implements InstanceMethodsAroundInterceptor {
@Override
public void beforeMethod(final EnhancedInstance objInst, final Method method, final Object[] allArguments,
final Class<?>[] argumentsTypes, final MethodInterceptResult result) {
- DefaultHttpRequest httpRequest = (DefaultHttpRequest) allArguments[1];
+ HttpRequest httpRequest = (HttpRequest) allArguments[1];
Review Comment:
CI fails due to this.
##########
apm-sniffer/apm-sdk-plugin/apm-armeria-plugins/apm-armeria-0.99.x-plugin/src/main/resources/skywalking-plugin.def:
##########
@@ -0,0 +1,17 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+armeria-0.99.0=org.apache.skywalking.apm.plugin.armeria.define.Armeria099ClientInstrumentation
Review Comment:
This is a new plugin, `plugin-list.md` and `supported-list.md` should be updated.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] zhangrm commented on pull request #459: Adapt Armeria's apm-sdk-plugin (client and server) version from 0.98x to the latest version 1.22 x
Posted by "zhangrm (via GitHub)" <gi...@apache.org>.
zhangrm commented on PR #459:
URL: https://github.com/apache/skywalking-java/pull/459#issuecomment-1437301892
> Could you explain what is the difference between the new case and existing Armeria cases? And new case should be added to GitHub action control file.
Since Armeria version 0.99.0, the new ServerBuilder() construction method has been removedοΌuse the Server. builder () method instead
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] zhangrm commented on pull request #459: Adapt Armeria's apm-sdk-plugin (client and server) version from 0.98x to the latest version 1.21. x
Posted by "zhangrm (via GitHub)" <gi...@apache.org>.
zhangrm commented on PR #459:
URL: https://github.com/apache/skywalking-java/pull/459#issuecomment-1436262714
> Notice, you should move the other two Armeria plugins into the same place.
We've talked about this, put up another issue to move those two packages.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] zhangrm commented on pull request #459: Adapt Armeria's apm-sdk-plugin (client and server) version from 0.98x to the latest version 1.21. x
Posted by "zhangrm (via GitHub)" <gi...@apache.org>.
zhangrm commented on PR #459:
URL: https://github.com/apache/skywalking-java/pull/459#issuecomment-1436256383
I'll add the test cases as soon as possible, this week.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] zhangrm commented on a diff in pull request #459: Adapt Armeria's apm-sdk-plugin (client and server) version from 0.98x to the latest version 1.22 x
Posted by "zhangrm (via GitHub)" <gi...@apache.org>.
zhangrm commented on code in PR #459:
URL: https://github.com/apache/skywalking-java/pull/459#discussion_r1117218129
##########
apm-sniffer/apm-sdk-plugin/armeria-0.85.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/armeria/Armeria085ServerInterceptor.java:
##########
@@ -32,13 +31,15 @@
import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.MethodInterceptResult;
import org.apache.skywalking.apm.network.trace.component.ComponentsDefine;
+import java.lang.reflect.Method;
+
@SuppressWarnings("unused") // actually used
public class Armeria085ServerInterceptor implements InstanceMethodsAroundInterceptor {
@Override
public void beforeMethod(final EnhancedInstance objInst, final Method method, final Object[] allArguments,
final Class<?>[] argumentsTypes, final MethodInterceptResult result) {
- DefaultHttpRequest httpRequest = (DefaultHttpRequest) allArguments[1];
+ HttpRequest httpRequest = (HttpRequest) allArguments[1];
Review Comment:
> I think you don't need to change this. As this plugin should only work when version < 0.99. If not, it means you need another witness class in 0.85-0.99 plugin.
I checked the Armeria source code. The `com.linecorp.armeria.common.DefaultHttpRequest` class in versions 0.85-0.98 has been marked as `Deprecated`, and the version after 0.99 has been removed. Its parent class, `com.linecorp.armeria.common.HttpRequest` has always existed in versions 0.85-1.21 and its behavior has not changed, and it is fully applicable in Interceptor. So I think `HttpRequest` can be used to replace `DefaultHttpRequest` after completion
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] wu-sheng commented on pull request #459: Adapt Armeria's plugins to the latest version 1.22.x
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on PR #459:
URL: https://github.com/apache/skywalking-java/pull/459#issuecomment-1445345564
@kezhenxu94 Could you take a look?
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] wu-sheng commented on pull request #459: Adapt Armeria's apm-sdk-plugin (client and server) version from 0.98x to the latest version 1.21. x
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on PR #459:
URL: https://github.com/apache/skywalking-java/pull/459#issuecomment-1436260096
> I'll add the test cases as soon as possible, this week.
If end users are not sensitive to the latest update(such as no user-side API change), you could just add versions to https://github.com/apache/skywalking-java/blob/main/test/plugin/scenarios/armeria-0.96plus-scenario/support-version.list
If it changes, you have to write new one.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] wu-sheng commented on pull request #459: Adapt Armeria's apm-sdk-plugin (client and server) version from 0.98x to the latest version 1.21. x
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on PR #459:
URL: https://github.com/apache/skywalking-java/pull/459#issuecomment-1435836646
Do you need to write new Armeria case to test or just add more versions to existing test case?
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] zhangrm commented on a diff in pull request #459: Adapt Armeria's apm-sdk-plugin (client and server) version from 0.98x to the latest version 1.22 x
Posted by "zhangrm (via GitHub)" <gi...@apache.org>.
zhangrm commented on code in PR #459:
URL: https://github.com/apache/skywalking-java/pull/459#discussion_r1117122275
##########
apm-sniffer/apm-sdk-plugin/apm-armeria-plugins/apm-armeria-0.99.x-plugin/src/main/resources/skywalking-plugin.def:
##########
@@ -0,0 +1,17 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+armeria-0.99.0=org.apache.skywalking.apm.plugin.armeria.define.Armeria099ClientInstrumentation
Review Comment:
Plugin-list.md and supported-list.md have been updated
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] zhangrm commented on pull request #459: Adapt Armeria's apm-sdk-plugin (client and server) version from 0.98x to the latest version 1.22 x
Posted by "zhangrm (via GitHub)" <gi...@apache.org>.
zhangrm commented on PR #459:
URL: https://github.com/apache/skywalking-java/pull/459#issuecomment-1437324157
After careful consideration, I think I can modify the initial method of ServerBuilder in the armeria-0.96plus-scenario module, so that I don't need to add new modules
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] wu-sheng commented on pull request #459: Adapt Armeria's apm-sdk-plugin (client and server) version from 0.98x to the latest version 1.21. x
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on PR #459:
URL: https://github.com/apache/skywalking-java/pull/459#issuecomment-1436264253
> > Notice, you should move the other two Armeria plugins into the same place.
>
> We've talked about this, put up another issue to move those two packages.
Yes, usually changing structure comes first then adding new, that is why I reminded.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] wu-sheng commented on pull request #459: Adapt Armeria's apm-sdk-plugin (client and server) version from 0.98x to the latest version 1.22 x
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on PR #459:
URL: https://github.com/apache/skywalking-java/pull/459#issuecomment-1437295082
Could you explain what is the difference between the new case and existing Armeria cases? And new case should be added to GitHub action control file.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org