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/14 04:42:09 UTC
[GitHub] [skywalking] kezhenxu94 opened a new pull request, #8874: Fix event can't split service ID into 2 parts
kezhenxu94 opened a new pull request, #8874:
URL: https://github.com/apache/skywalking/pull/8874
<!--
⚠️ 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 #8864 .
- [x] Update the [`CHANGES` log](https://github.com/apache/skywalking/blob/changelog/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] wu-sheng commented on pull request #8874: Fix event can't split service ID into 2 parts
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on PR #8874:
URL: https://github.com/apache/skywalking/pull/8874#issuecomment-1098729589
One tip, Event toBuilder and then build could be a performance impact. I would not concern it as we should not face too many events concurrently.
--
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 #8874: Fix event can't split service ID into 2 parts
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on PR #8874:
URL: https://github.com/apache/skywalking/pull/8874#issuecomment-1098715207
I think it is worth to avoid this, the mainly concern is in ElasticSearch case, it could create a new and never removed index, and data could keep increasing, which leads crash.
Time bucket before 19700101 is definitely not legal. 😀
If you don't want to hide error, print warning logs in receiver should be 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.
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 #8874: Fix event can't split service ID into 2 parts
Posted by GitBox <gi...@apache.org>.
wu-sheng merged PR #8874:
URL: https://github.com/apache/skywalking/pull/8874
--
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 a diff in pull request #8874: Fix event can't split service ID into 2 parts
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on code in PR #8874:
URL: https://github.com/apache/skywalking/pull/8874#discussion_r850093108
##########
oap-server/analyzer/event-analyzer/src/main/java/org/apache/skywalking/oap/server/analyzer/event/EventAnalyzerServiceImpl.java:
##########
@@ -34,8 +36,20 @@ public class EventAnalyzerServiceImpl implements EventAnalyzerService, EventAnal
@Override
public void analyze(final Event event) {
+ final Event.Builder eb = event.toBuilder();
+ if (event.getStartTime() <= 0) {
+ log.warn("Event start time {} is invalid, it will be set to current time, eventId: {}",
+ event.getStartTime(), event.getUuid());
+ eb.setStartTime(System.currentTimeMillis());
+ }
+ if (event.getEndTime() <= 0) {
+ log.warn("Event end time {} is invalid, it will be set to current time, eventId: {}",
+ event.getEndTime(), event.getUuid());
+ eb.setEndTime(System.currentTimeMillis());
+ }
+
Review Comment:
The think the rule should be either start or end is legal
And these two variables here are timestamps, not time bucket, right?
##########
oap-server/analyzer/event-analyzer/src/main/java/org/apache/skywalking/oap/server/analyzer/event/EventAnalyzerServiceImpl.java:
##########
@@ -34,8 +36,20 @@ public class EventAnalyzerServiceImpl implements EventAnalyzerService, EventAnal
@Override
public void analyze(final Event event) {
+ final Event.Builder eb = event.toBuilder();
+ if (event.getStartTime() <= 0) {
+ log.warn("Event start time {} is invalid, it will be set to current time, eventId: {}",
+ event.getStartTime(), event.getUuid());
+ eb.setStartTime(System.currentTimeMillis());
+ }
+ if (event.getEndTime() <= 0) {
+ log.warn("Event end time {} is invalid, it will be set to current time, eventId: {}",
+ event.getEndTime(), event.getUuid());
+ eb.setEndTime(System.currentTimeMillis());
+ }
+
Review Comment:
I remember you mentioned, start time and endpoint could be assigned separatedly in two requests?
This warning maybe trigger false warning?
--
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] kezhenxu94 commented on pull request #8874: Fix event can't split service ID into 2 parts
Posted by GitBox <gi...@apache.org>.
kezhenxu94 commented on PR #8874:
URL: https://github.com/apache/skywalking/pull/8874#issuecomment-1098718480
> I think it is worth to avoid this, the mainly concern is in ElasticSearch case, it could create a new and never removed index, and data could keep increasing, which leads crash.
This makes total 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.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] kezhenxu94 commented on pull request #8874: Fix event can't split service ID into 2 parts
Posted by GitBox <gi...@apache.org>.
kezhenxu94 commented on PR #8874:
URL: https://github.com/apache/skywalking/pull/8874#issuecomment-1098713567
> I suggest you add a fail safe of making sure the time bucket not null. If it is not set correctly, use the server time to fill it. WDYT?
Time bucket is always not null, the startTime and endTime of an event are always not null, they are all `long` primitives.
In this case the problem is that event exporter set it wrongly, with a negative value (like -6795364578871345152), do you think I should "correct" this if users deliberately set it wrong? IMO this would hide something that is really wrong, for example, in this case, the event exporter needs a fix. Adding the fail safe hides this problem
--
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 a diff in pull request #8874: Fix event can't split service ID into 2 parts
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on code in PR #8874:
URL: https://github.com/apache/skywalking/pull/8874#discussion_r850078490
##########
oap-server/server-core/src/main/java/org/apache/skywalking/oap/server/core/analysis/metrics/Metrics.java:
##########
@@ -102,7 +102,7 @@ public long toTimeBucketInHour() {
if (isMinuteBucket()) {
return timeBucket / 100;
} else {
- throw new IllegalStateException("Current time bucket is not in minute dimensionality");
+ throw new IllegalStateException("Current time bucket is not in minute dimensionality: " + timeBucket);
Review Comment:
I think you could see the time bucket from index-0? It should mean no value of time bucket, right?
##########
oap-server/server-core/src/main/java/org/apache/skywalking/oap/server/core/analysis/metrics/Metrics.java:
##########
@@ -102,7 +102,7 @@ public long toTimeBucketInHour() {
if (isMinuteBucket()) {
return timeBucket / 100;
} else {
- throw new IllegalStateException("Current time bucket is not in minute dimensionality");
+ throw new IllegalStateException("Current time bucket is not in minute dimensionality: " + timeBucket);
Review Comment:
Is this log really helpful?
--
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 #8874: Fix event can't split service ID into 2 parts
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on PR #8874:
URL: https://github.com/apache/skywalking/pull/8874#issuecomment-1098708326
I suggest you add a fail safe of making sure the time bucket not null. If it is not set correctly, use the server time to fill it. WDYT?
--
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] kezhenxu94 commented on pull request #8874: Fix event can't split service ID into 2 parts
Posted by GitBox <gi...@apache.org>.
kezhenxu94 commented on PR #8874:
URL: https://github.com/apache/skywalking/pull/8874#issuecomment-1098707653
> Should we close that issue? Is the time bucket issue addressed?
That was caused by kubernetes event exporter, should be fixed in another repo
--
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] kezhenxu94 commented on a diff in pull request #8874: Fix event can't split service ID into 2 parts
Posted by GitBox <gi...@apache.org>.
kezhenxu94 commented on code in PR #8874:
URL: https://github.com/apache/skywalking/pull/8874#discussion_r850094346
##########
oap-server/analyzer/event-analyzer/src/main/java/org/apache/skywalking/oap/server/analyzer/event/EventAnalyzerServiceImpl.java:
##########
@@ -34,8 +36,20 @@ public class EventAnalyzerServiceImpl implements EventAnalyzerService, EventAnal
@Override
public void analyze(final Event event) {
+ final Event.Builder eb = event.toBuilder();
+ if (event.getStartTime() <= 0) {
+ log.warn("Event start time {} is invalid, it will be set to current time, eventId: {}",
+ event.getStartTime(), event.getUuid());
+ eb.setStartTime(System.currentTimeMillis());
+ }
+ if (event.getEndTime() <= 0) {
+ log.warn("Event end time {} is invalid, it will be set to current time, eventId: {}",
+ event.getEndTime(), event.getUuid());
+ eb.setEndTime(System.currentTimeMillis());
+ }
+
Review Comment:
> The think the rule should be either start or end is legal
Ha, right.
> And these two variables here are timestamps, not time bucket, right?
Yes. They are timestamp here, haven't been converted to time bucket yet,
--
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 a diff in pull request #8874: Fix event can't split service ID into 2 parts
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on code in PR #8874:
URL: https://github.com/apache/skywalking/pull/8874#discussion_r850098589
##########
oap-server/analyzer/event-analyzer/src/main/java/org/apache/skywalking/oap/server/analyzer/event/EventAnalyzerServiceImpl.java:
##########
@@ -34,8 +36,19 @@ public class EventAnalyzerServiceImpl implements EventAnalyzerService, EventAnal
@Override
public void analyze(final Event event) {
+ final Event.Builder eb = event.toBuilder();
+ if (event.getStartTime() <= 0 && event.getEndTime() <= 0) {
+ log.warn(
+ "Event start time {} and end time are both invalid, they will be set to current time, eventId: {}",
Review Comment:
```suggestion
"Event start time {} and end time {} are both invalid, they will be set to current time, eventId: {}",
```
##########
oap-server/analyzer/event-analyzer/src/main/java/org/apache/skywalking/oap/server/analyzer/event/EventAnalyzerServiceImpl.java:
##########
@@ -34,8 +36,19 @@ public class EventAnalyzerServiceImpl implements EventAnalyzerService, EventAnal
@Override
public void analyze(final Event event) {
+ final Event.Builder eb = event.toBuilder();
+ if (event.getStartTime() <= 0 && event.getEndTime() <= 0) {
+ log.warn(
+ "Event start time {} and end time are both invalid, they will be set to current time, eventId: {}",
Review Comment:
Miss 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] wu-sheng commented on pull request #8874: Fix event can't split service ID into 2 parts
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on PR #8874:
URL: https://github.com/apache/skywalking/pull/8874#issuecomment-1098704204
Should we close that issue? Is the time bucket issue addressed?
--
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