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 2022/04/01 08:43:44 UTC
[GitHub] [skywalking-java] zhyyu opened a new pull request #135: add SAMPLE_AVG_CPU_LOAD_LIMIT config
zhyyu opened a new pull request #135:
URL: https://github.com/apache/skywalking-java/pull/135
<!--
⚠️ 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 👆 ==== -->
- [x] If this pull request closes/resolves/fixes an existing issue, replace the issue number. Closes #<[8773](https://github.com/apache/skywalking/issues/8773)>.
- [ ] 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] zhyyu commented on pull request #135: add SAMPLE_AVG_CPU_LOAD_LIMIT config
Posted by GitBox <gi...@apache.org>.
zhyyu commented on pull request #135:
URL: https://github.com/apache/skywalking-java/pull/135#issuecomment-1085711040
> Take a look at how `TraceIgnoreExtendService` extends from `SamplingService`. I think it would be more elegant. This doesn't have to change from the core level.
>
> Another thing is, you add another thread just for CPU usage, which concerns me much. The agent is very sensitive about a new thread. It should be more reasonable to ask `JVMService` holds the latest CPU value, and provide a method for this new plugin.
Ok, I will try it later using `JVMService,` add a new plugin like `trace-ignore-plugin`.
--
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 change in pull request #135: add SAMPLE_AVG_CPU_LOAD_LIMIT config
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on a change in pull request #135:
URL: https://github.com/apache/skywalking-java/pull/135#discussion_r841049719
##########
File path: apm-sniffer/apm-agent-core/src/main/java/org/apache/skywalking/apm/agent/core/jvm/JVMService.java
##########
@@ -104,8 +106,16 @@ public void run() {
jvmBuilder.setClazz(ClassProvider.INSTANCE.getClassMetrics());
sender.offer(jvmBuilder.build());
+
+ // refresh cpu usage percent
+ cpuUsagePercent = jvmBuilder.getCpu().getUsagePercent();
Review comment:
You are using a `#build`-ed builder to read data. I think it is much safe to read it from `build` result or create a local variable to host the value.
--
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] zhyyu commented on a change in pull request #135: add SAMPLE_AVG_CPU_LOAD_LIMIT config
Posted by GitBox <gi...@apache.org>.
zhyyu commented on a change in pull request #135:
URL: https://github.com/apache/skywalking-java/pull/135#discussion_r841047972
##########
File path: apm-sniffer/apm-agent-core/src/main/java/org/apache/skywalking/apm/agent/core/jvm/JVMService.java
##########
@@ -104,8 +106,16 @@ public void run() {
jvmBuilder.setClazz(ClassProvider.INSTANCE.getClassMetrics());
sender.offer(jvmBuilder.build());
+
+ // refresh cpu usage percent
+ cpuUsagePercent = jvmBuilder.getCpu().getUsagePercent();
Review comment:
What issue do you think will cause, maybe my debug way miss it.
--
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 #135: add SAMPLE_AVG_CPU_LOAD_LIMIT config
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on pull request #135:
URL: https://github.com/apache/skywalking-java/pull/135#issuecomment-1086595896
I have commented on the codes, and I think you haven't tried this in the real case, please do so before submitting codes.
--
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 change in pull request #135: add SAMPLE_AVG_CPU_LOAD_LIMIT config
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on a change in pull request #135:
URL: https://github.com/apache/skywalking-java/pull/135#discussion_r841047196
##########
File path: apm-sniffer/optional-plugins/pom.xml
##########
@@ -44,6 +44,7 @@
<modules>
<module>optional-spring-plugins</module>
<module>trace-ignore-plugin</module>
+ <module>trace-cpu-limit-plugin</module>
Review comment:
Add the last of the list.
--
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 change in pull request #135: add SAMPLE_AVG_CPU_LOAD_LIMIT config
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on a change in pull request #135:
URL: https://github.com/apache/skywalking-java/pull/135#discussion_r841046460
##########
File path: apm-sniffer/apm-agent-core/src/main/java/org/apache/skywalking/apm/agent/core/jvm/JVMService.java
##########
@@ -104,8 +106,16 @@ public void run() {
jvmBuilder.setClazz(ClassProvider.INSTANCE.getClassMetrics());
sender.offer(jvmBuilder.build());
+
+ // refresh cpu usage percent
+ cpuUsagePercent = jvmBuilder.getCpu().getUsagePercent();
Review comment:
I think `getUsagePercent` again would cause an issue. Have you tested?
--
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] zhyyu commented on a change in pull request #135: add SAMPLE_AVG_CPU_LOAD_LIMIT config
Posted by GitBox <gi...@apache.org>.
zhyyu commented on a change in pull request #135:
URL: https://github.com/apache/skywalking-java/pull/135#discussion_r841047707
##########
File path: apm-sniffer/optional-plugins/trace-cpu-limit-plugin/src/main/java/org/apache/skywalking/apm/plugin/trace/cpu/limit/conf/CpuLimitConfigInitializer.java
##########
@@ -0,0 +1,92 @@
+/*
+ * 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.
+ *
+ */
+
+package org.apache.skywalking.apm.plugin.trace.cpu.limit.conf;
+
+import java.io.File;
+import java.io.FileInputStream;
+import java.io.FileNotFoundException;
+import java.io.InputStream;
+import java.util.Map;
+import java.util.Properties;
+import org.apache.skywalking.apm.agent.core.boot.AgentPackageNotFoundException;
+import org.apache.skywalking.apm.agent.core.boot.AgentPackagePath;
+import org.apache.skywalking.apm.agent.core.conf.ConfigNotFoundException;
+import org.apache.skywalking.apm.agent.core.logging.api.ILog;
+import org.apache.skywalking.apm.agent.core.logging.api.LogManager;
+import org.apache.skywalking.apm.util.ConfigInitializer;
+import org.apache.skywalking.apm.util.PropertyPlaceholderHelper;
+
+public class CpuLimitConfigInitializer {
Review comment:
ok
--
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 #135: add SAMPLE_AVG_CPU_LOAD_LIMIT config
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on pull request #135:
URL: https://github.com/apache/skywalking-java/pull/135#issuecomment-1086600025
As you are adding a CPU based sampler, so, you should verify whether this is really working.
The code review only could find out some issues, but can't approve this could work as expected. Also, this plugin doesn't like other plugins, we can't test it automatically.
So, please try to establish a runtime env to verify this plugin.
--
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 change in pull request #135: add SAMPLE_AVG_CPU_LOAD_LIMIT config
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on a change in pull request #135:
URL: https://github.com/apache/skywalking-java/pull/135#discussion_r841046740
##########
File path: apm-sniffer/optional-plugins/trace-cpu-limit-plugin/pom.xml
##########
@@ -0,0 +1,45 @@
+<!--
+ ~ 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.
+ ~
+ -->
+
+<project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
+ <parent>
+ <artifactId>optional-plugins</artifactId>
+ <groupId>org.apache.skywalking</groupId>
+ <version>8.10.0-SNAPSHOT</version>
+ </parent>
+ <modelVersion>4.0.0</modelVersion>
+
+ <artifactId>apm-trace-cpu-limit-plugin</artifactId>
Review comment:
```suggestion
<artifactId>trace-sampler-cpu-policy-plugin</artifactId>
```
--
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] zhyyu removed a comment on pull request #135: add SAMPLE_AVG_CPU_LOAD_LIMIT config
Posted by GitBox <gi...@apache.org>.
zhyyu removed a comment on pull request #135:
URL: https://github.com/apache/skywalking-java/pull/135#issuecomment-1085707455
>
Ok, I will try it later
--
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] zhyyu commented on pull request #135: add SAMPLE_AVG_CPU_LOAD_LIMIT config
Posted by GitBox <gi...@apache.org>.
zhyyu commented on pull request #135:
URL: https://github.com/apache/skywalking-java/pull/135#issuecomment-1085707455
>
Ok, I will try it later
--
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 change in pull request #135: add SAMPLE_AVG_CPU_LOAD_LIMIT config
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on a change in pull request #135:
URL: https://github.com/apache/skywalking-java/pull/135#discussion_r841047238
##########
File path: apm-sniffer/config/apm-trace-cpu-limit-plugin.config
##########
@@ -0,0 +1,23 @@
+# 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.
+
+# If the operation name of the first span is matching, this segment should be ignored
+# ant path match style
+# /path/? Match any single character
+# /path/* Match any number of characters
+# /path/** Match any number of characters and support multilevel directories
+# Multiple path comma separation, like trace.ignore_path=/eureka/**,/consul/**
Review comment:
Copy/paste 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.
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] zhyyu commented on pull request #135: add SAMPLE_AVG_CPU_LOAD_LIMIT config
Posted by GitBox <gi...@apache.org>.
zhyyu commented on pull request #135:
URL: https://github.com/apache/skywalking-java/pull/135#issuecomment-1086598769
> I have commented on the codes, and I think you haven't tried this in the real case, please do so before submitting codes.
I just used idea to debug the agent code, maybe not production scenario... But it do sent the segment data to OAP. I will change the code according to the comment above.
--
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 change in pull request #135: add SAMPLE_AVG_CPU_LOAD_LIMIT config
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on a change in pull request #135:
URL: https://github.com/apache/skywalking-java/pull/135#discussion_r841047149
##########
File path: apm-sniffer/optional-plugins/trace-cpu-limit-plugin/src/main/java/org/apache/skywalking/apm/plugin/trace/cpu/limit/conf/CpuLimitConfigInitializer.java
##########
@@ -0,0 +1,92 @@
+/*
+ * 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.
+ *
+ */
+
+package org.apache.skywalking.apm.plugin.trace.cpu.limit.conf;
+
+import java.io.File;
+import java.io.FileInputStream;
+import java.io.FileNotFoundException;
+import java.io.InputStream;
+import java.util.Map;
+import java.util.Properties;
+import org.apache.skywalking.apm.agent.core.boot.AgentPackageNotFoundException;
+import org.apache.skywalking.apm.agent.core.boot.AgentPackagePath;
+import org.apache.skywalking.apm.agent.core.conf.ConfigNotFoundException;
+import org.apache.skywalking.apm.agent.core.logging.api.ILog;
+import org.apache.skywalking.apm.agent.core.logging.api.LogManager;
+import org.apache.skywalking.apm.util.ConfigInitializer;
+import org.apache.skywalking.apm.util.PropertyPlaceholderHelper;
+
+public class CpuLimitConfigInitializer {
Review comment:
This is the wrong way to initialize a simple value config on your own initializer. The ignore plugin has its own defined config file, so, it does in this way. You just need a threshold, so, you could use the standard way and share the `agent.config` to load the feature.
Take a look at `SpringMVCPluginConfig`.
--
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] zhyyu commented on a change in pull request #135: add SAMPLE_AVG_CPU_LOAD_LIMIT config
Posted by GitBox <gi...@apache.org>.
zhyyu commented on a change in pull request #135:
URL: https://github.com/apache/skywalking-java/pull/135#discussion_r841047631
##########
File path: apm-sniffer/apm-agent-core/src/main/java/org/apache/skywalking/apm/agent/core/jvm/JVMService.java
##########
@@ -104,8 +106,16 @@ public void run() {
jvmBuilder.setClazz(ClassProvider.INSTANCE.getClassMetrics());
sender.offer(jvmBuilder.build());
+
+ // refresh cpu usage percent
+ cpuUsagePercent = jvmBuilder.getCpu().getUsagePercent();
Review comment:
I have tested it using debug, it returned a val less than 100.
--
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 #135: add SAMPLE_AVG_CPU_LOAD_LIMIT config
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on pull request #135:
URL: https://github.com/apache/skywalking-java/pull/135#issuecomment-1085638278
Take a look at how `TraceIgnoreExtendService` extends from `SamplingService`. I think it would be more elegant.
This doesn't have to change from the core level.
Another thing is, you add another thread just for CPU usage, which concerns me much. The agent is very sensitive about a new thread. It should be more reasonable to ask `JVMService` holds the latest CPU value, and provide a method for this new plugin.
--
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