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/10/09 03:54:06 UTC
[GitHub] [skywalking] xzyJavaX opened a new pull request, #9744: optimize the creation conditions of profiling task
xzyJavaX opened a new pull request, #9744:
URL: https://github.com/apache/skywalking/pull/9744
<!--
⚠️ 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 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/blob/master/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 #[9742](https://github.com/apache/skywalking/issues/9742)
- [x] Update the [`CHANGES` log](https://github.com/apache/skywalking/blob/master/docs/en/changes/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] mrproliu commented on pull request #9744: optimize the creation conditions of profiling task
Posted by GitBox <gi...@apache.org>.
mrproliu commented on PR #9744:
URL: https://github.com/apache/skywalking/pull/9744#issuecomment-1272479769
> query.must(Query.range(ProfileTaskRecord.TIME_BUCKET).gte(endTimeBucket));
The `TIME_BUCKET` is usually the same as `START_TIME `, we should ignore it.
> If the data can be found, it indicates that an overlay has occurred, and the task should not be created at this time.
You need to recheck again. If the previous task is already finished, the creation process should 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.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] mrproliu commented on a diff in pull request #9744: optimize the creation conditions of profiling task
Posted by GitBox <gi...@apache.org>.
mrproliu commented on code in PR #9744:
URL: https://github.com/apache/skywalking/pull/9744#discussion_r990883444
##########
oap-server/server-core/src/main/java/org/apache/skywalking/oap/server/core/profiling/trace/ProfileTaskMutationService.java:
##########
@@ -140,14 +140,16 @@ private String checkDataSuccess(final String serviceId,
// Each service can monitor up to 1 endpoints during the execution of tasks
long startTimeBucket = TimeBucket.getMinuteTimeBucket(monitorStartTime);
- long endTimeBucket = TimeBucket.getMinuteTimeBucket(monitorEndTime);
final List<ProfileTask> alreadyHaveTaskList = getProfileTaskDAO().getTaskList(
- serviceId, null, startTimeBucket, endTimeBucket, 1);
+ serviceId, null, startTimeBucket, null, null);
Review Comment:
I think `1` is enough. Usually, you just need to check if the most recent task has overlap or not.
--
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] mrproliu commented on pull request #9744: optimize the creation conditions of profiling task
Posted by GitBox <gi...@apache.org>.
mrproliu commented on PR #9744:
URL: https://github.com/apache/skywalking/pull/9744#issuecomment-1272451235
Thanks for your contribution, I have two questions:
1. I don't think deleting the condition is a good idea, because we have other queries that need to use the end time bucket filter(such as cache timer). If you don't need the end time bucket filter, just set it as `null`.
2. If you ignore the end time bucket when creating a task, then the profiling will generally be created successfully(if the start time not be set), It's not right.
--
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] xzyJavaX commented on a diff in pull request #9744: optimize the creation conditions of profiling task
Posted by GitBox <gi...@apache.org>.
xzyJavaX commented on code in PR #9744:
URL: https://github.com/apache/skywalking/pull/9744#discussion_r990881077
##########
oap-server/server-core/src/main/java/org/apache/skywalking/oap/server/core/profiling/trace/ProfileTaskMutationService.java:
##########
@@ -140,14 +140,16 @@ private String checkDataSuccess(final String serviceId,
// Each service can monitor up to 1 endpoints during the execution of tasks
long startTimeBucket = TimeBucket.getMinuteTimeBucket(monitorStartTime);
- long endTimeBucket = TimeBucket.getMinuteTimeBucket(monitorEndTime);
final List<ProfileTask> alreadyHaveTaskList = getProfileTaskDAO().getTaskList(
- serviceId, null, startTimeBucket, endTimeBucket, 1);
+ serviceId, null, startTimeBucket, null, null);
Review Comment:
sorry, I used to think of TIME_BUCKE as END_TIME.
What do you think the "limit" should be set to.
--
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] wu-sheng commented on pull request #9744: optimize the creation conditions of profiling task
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on PR #9744:
URL: https://github.com/apache/skywalking/pull/9744#issuecomment-1272449519
Could you explain why removing the end-time condition helps?
--
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] wu-sheng commented on pull request #9744: optimize the creation conditions of profiling task
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on PR #9744:
URL: https://github.com/apache/skywalking/pull/9744#issuecomment-1272478013
> Continue to judge after the alreadyHaveTaskList is found,if alreadyHaveTaskList isEmpty
This seems wrong from a grammar perspective. If a method named `alreadyHaveTaskList` return null, it should have no further query, right?
--
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] wu-sheng commented on pull request #9744: optimize the creation conditions of profiling task
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on PR #9744:
URL: https://github.com/apache/skywalking/pull/9744#issuecomment-1272479671
I think you could `getProfileTaskDAO().getTaskList()` to get a basic list, which should not be long, and just filter it in the memory.
--
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] wu-sheng commented on pull request #9744: optimize the creation conditions of profiling task
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on PR #9744:
URL: https://github.com/apache/skywalking/pull/9744#issuecomment-1272451581
I think we should build a strict condition to avoid overlap.
--
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] sonatype-lift[bot] commented on pull request #9744: optimize the creation conditions of profiling task
Posted by GitBox <gi...@apache.org>.
sonatype-lift[bot] commented on PR #9744:
URL: https://github.com/apache/skywalking/pull/9744#issuecomment-1272453677
:warning: **6 God Classes** were detected by Lift in this project. [Visit the Lift web console](https://lift.sonatype.com/results/github.com/apache/skywalking/01GEXE1V2RM24TJS6ACS0PS4S7?tab=technical-debt&utm_source=github.com&utm_campaign=lift-comment&utm_content=apache\%20skywalking) for more details.
--
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] wu-sheng merged pull request #9744: optimize the creation conditions of profiling task
Posted by GitBox <gi...@apache.org>.
wu-sheng merged PR #9744:
URL: https://github.com/apache/skywalking/pull/9744
--
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] xzyJavaX commented on a diff in pull request #9744: optimize the creation conditions of profiling task
Posted by GitBox <gi...@apache.org>.
xzyJavaX commented on code in PR #9744:
URL: https://github.com/apache/skywalking/pull/9744#discussion_r990880757
##########
oap-server/server-core/src/main/java/org/apache/skywalking/oap/server/core/profiling/trace/ProfileTaskMutationService.java:
##########
@@ -140,14 +140,16 @@ private String checkDataSuccess(final String serviceId,
// Each service can monitor up to 1 endpoints during the execution of tasks
long startTimeBucket = TimeBucket.getMinuteTimeBucket(monitorStartTime);
- long endTimeBucket = TimeBucket.getMinuteTimeBucket(monitorEndTime);
final List<ProfileTask> alreadyHaveTaskList = getProfileTaskDAO().getTaskList(
- serviceId, null, startTimeBucket, endTimeBucket, 1);
+ serviceId, null, startTimeBucket, null, null);
Review Comment:
sorry, I used to think of TIME_BUCKE as END_TIME.
What do you think the "limit" should be set to.
--
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] wu-sheng commented on pull request #9744: optimize the creation conditions of profiling task
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on PR #9744:
URL: https://github.com/apache/skywalking/pull/9744#issuecomment-1272733572
3 ES relative profiling e2e fail. I just rerun to see whether they could pass, or there are some issues of this change.
--
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] mrproliu commented on pull request #9744: optimize the creation conditions of profiling task
Posted by GitBox <gi...@apache.org>.
mrproliu commented on PR #9744:
URL: https://github.com/apache/skywalking/pull/9744#issuecomment-1272453567
I think a good way to avoid overlap need following these steps:
1. find the recent profiling task from DB. An example SQL: `startBucket <= ${creating task end time}`.
2. Check the creating task start/end time is in the recent profiling task monitoring range.
--
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] xzyJavaX commented on pull request #9744: optimize the creation conditions of profiling task
Posted by GitBox <gi...@apache.org>.
xzyJavaX commented on PR #9744:
URL: https://github.com/apache/skywalking/pull/9744#issuecomment-1272476454
I think this should be feasible:
Continue to judge after the alreadyHaveTaskList is found,if alreadyHaveTaskList isEmpty,continue to query DB,query condition is
query.must(Query.range(ProfileTaskRecord.START_TIME).lte(endTimeBucket));
query.must(Query.range(ProfileTaskRecord.TIME_BUCKET).gte(endTimeBucket));
If the data can be found, it indicates that an overlay has occurred, and the task should not be created at this time.
--
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] mrproliu commented on a diff in pull request #9744: optimize the creation conditions of profiling task
Posted by GitBox <gi...@apache.org>.
mrproliu commented on code in PR #9744:
URL: https://github.com/apache/skywalking/pull/9744#discussion_r990788409
##########
oap-server/server-core/src/main/java/org/apache/skywalking/oap/server/core/profiling/trace/ProfileTaskMutationService.java:
##########
@@ -140,14 +140,16 @@ private String checkDataSuccess(final String serviceId,
// Each service can monitor up to 1 endpoints during the execution of tasks
long startTimeBucket = TimeBucket.getMinuteTimeBucket(monitorStartTime);
- long endTimeBucket = TimeBucket.getMinuteTimeBucket(monitorEndTime);
final List<ProfileTask> alreadyHaveTaskList = getProfileTaskDAO().getTaskList(
- serviceId, null, startTimeBucket, endTimeBucket, 1);
+ serviceId, null, startTimeBucket, null, null);
Review Comment:
1. If you use the start time to filter, then you may not get any data. For example, you create a 10 min task at 10:00, then when you create a new task at 10:05, then you cannot get the existing task. Am I right?
2. You should limit the profiling task count.
--
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] lujiajing1126 commented on a diff in pull request #9744: optimize the creation conditions of profiling task
Posted by GitBox <gi...@apache.org>.
lujiajing1126 commented on code in PR #9744:
URL: https://github.com/apache/skywalking/pull/9744#discussion_r990770793
##########
oap-server/server-core/src/main/java/org/apache/skywalking/oap/server/core/profiling/trace/ProfileTaskMutationService.java:
##########
@@ -140,14 +140,16 @@ private String checkDataSuccess(final String serviceId,
// Each service can monitor up to 1 endpoints during the execution of tasks
long startTimeBucket = TimeBucket.getMinuteTimeBucket(monitorStartTime);
- long endTimeBucket = TimeBucket.getMinuteTimeBucket(monitorEndTime);
final List<ProfileTask> alreadyHaveTaskList = getProfileTaskDAO().getTaskList(
- serviceId, null, startTimeBucket, endTimeBucket, 1);
+ serviceId, null, startTimeBucket, null, null);
if (CollectionUtils.isNotEmpty(alreadyHaveTaskList)) {
- // if any task time bucket in this range, means already have task, because time bucket is base on task end time
- return "current service already has monitor task execute at this time";
+ for (ProfileTask profileTask : alreadyHaveTaskList) {
+ if (profileTask.getStartTime() <= monitorEndTime) {
+ // if any task time bucket in this range, means already have task, because time bucket is base on task end time
Review Comment:
```suggestion
// if the startTime is less or equal than the endTime of the newly created task, i.e. there is overlap between two tasks, it is an invalid 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