You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@skywalking.apache.org by "Fine0830 (via GitHub)" <gi...@apache.org> on 2023/02/21 14:29:00 UTC
[GitHub] [skywalking-booster-ui] Fine0830 opened a new pull request, #239: feat: add a calculation for `cpm5d`
Fine0830 opened a new pull request, #239:
URL: https://github.com/apache/skywalking-booster-ui/pull/239
Reference https://github.com/apache/skywalking/pull/10420#issuecomment-1437282944
Video
https://user-images.githubusercontent.com/20871783/220372006-754054b5-e625-440f-aa96-94a5d4861aa8.mov
Signed-off-by: Qiuxia Fan <qi...@apache.org>
--
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-booster-ui] Fine0830 commented on pull request #239: feat: add a calculation for `cpm5d`
Posted by "Fine0830 (via GitHub)" <gi...@apache.org>.
Fine0830 commented on PR #239:
URL: https://github.com/apache/skywalking-booster-ui/pull/239#issuecomment-1439299630
Done
<img width="1374" alt="1" src="https://user-images.githubusercontent.com/20871783/220495130-d853e9e5-e942-426c-810d-40669b589c18.png">
--
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-booster-ui] toffentoffen commented on a diff in pull request #239: feat: add a calculation for `cpm5d`
Posted by "toffentoffen (via GitHub)" <gi...@apache.org>.
toffentoffen commented on code in PR #239:
URL: https://github.com/apache/skywalking-booster-ui/pull/239#discussion_r1113398015
##########
src/hooks/useMetricsProcessor.ts:
##########
@@ -398,15 +398,15 @@ export function aggregation(val: number, config: { calculation?: string }): numb
case Calculations.Apdex:
data = (val / 10000).toFixed(2);
break;
+ case Calculations.CPM5D:
+ data = (val / 100000).toFixed(2);
Review Comment:
Although I have to say, that for values `>=100000` it doesn't make sense to display always 5 decimals.
`(100000/100000).toFixed(5) == '1.00000'`
Maybe it should be "dynamic" if value >= 100000 display 2 decimals.
##########
src/hooks/useMetricsProcessor.ts:
##########
@@ -398,15 +398,15 @@ export function aggregation(val: number, config: { calculation?: string }): numb
case Calculations.Apdex:
data = (val / 10000).toFixed(2);
break;
+ case Calculations.CPM5D:
+ data = (val / 100000).toFixed(2);
Review Comment:
I think it is better as @wu-sheng suggested. Good catch.
My main reason is to allow converting it back. For instance if we have had only 1 call in a day:
How it will be displayed:
- `(69/100000).toFixed(2) == '0.00'`
- `(69/100000).toFixed(5) == '0.00069'
If the user, would like to know how many request they have had:
- `(69/100000).toFixed(2) * 1440 == 0`
- `(69/100000).toFixed(5) * 1440 == 0.9935999999999999` ~ 1 request
--
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-booster-ui] Fine0830 commented on a diff in pull request #239: feat: add a calculation for `cpm5d`
Posted by "Fine0830 (via GitHub)" <gi...@apache.org>.
Fine0830 commented on code in PR #239:
URL: https://github.com/apache/skywalking-booster-ui/pull/239#discussion_r1113750598
##########
src/views/dashboard/data.ts:
##########
@@ -309,7 +309,7 @@ export const CalculationOpts = [
value: "convertMilliseconds",
},
{ label: "Seconds to YYYY-MM-DD HH:mm:ss", value: "convertSeconds" },
- { label: "Precision is 2", value: "precision" },
+ { label: "CPM5D", value: "cpm5d" },
Review Comment:
What do you think of `Decrease 100000 times`? @wu-sheng @kezhenxu94
--
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-booster-ui] wu-sheng commented on a diff in pull request #239: feat: add a calculation for `cpm5d`
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on code in PR #239:
URL: https://github.com/apache/skywalking-booster-ui/pull/239#discussion_r1113174991
##########
src/hooks/useMetricsProcessor.ts:
##########
@@ -398,15 +398,15 @@ export function aggregation(val: number, config: { calculation?: string }): numb
case Calculations.Apdex:
data = (val / 10000).toFixed(2);
break;
+ case Calculations.CPM5D:
+ data = (val / 100000).toFixed(2);
Review Comment:
```suggestion
data = (val / 100000).toFixed(5);
```
I think it is better to keep 5 digit? @@toffentoffen
--
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-booster-ui] toffentoffen commented on a diff in pull request #239: feat: add a calculation for `cpm5d`
Posted by "toffentoffen (via GitHub)" <gi...@apache.org>.
toffentoffen commented on code in PR #239:
URL: https://github.com/apache/skywalking-booster-ui/pull/239#discussion_r1113389501
##########
src/hooks/useMetricsProcessor.ts:
##########
@@ -398,15 +398,15 @@ export function aggregation(val: number, config: { calculation?: string }): numb
case Calculations.Apdex:
data = (val / 10000).toFixed(2);
break;
+ case Calculations.CPM5D:
+ data = (val / 100000).toFixed(2);
Review Comment:
I think it is better as @wu-sheng suggested. Good catch.
My main reason is to allow converting it back. For instance if we have had only 1 call in a day:
How it will be displayed:
- `(69/100000).toFixed(2) == '0.00'`
- `(69/100000).toFixed(5) == '0.00069'
If the user, would like to know how many request they have had:
- `(69/100000).toFixed(2) * 1440` == 0`
- `(69/100000).toFixed(5) * 1440 == 0.9935999999999999` ~ 1 request
--
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-booster-ui] toffentoffen commented on a diff in pull request #239: feat: add a calculation for `cpm5d`
Posted by "toffentoffen (via GitHub)" <gi...@apache.org>.
toffentoffen commented on code in PR #239:
URL: https://github.com/apache/skywalking-booster-ui/pull/239#discussion_r1113389805
##########
src/hooks/useMetricsProcessor.ts:
##########
@@ -398,15 +398,15 @@ export function aggregation(val: number, config: { calculation?: string }): numb
case Calculations.Apdex:
data = (val / 10000).toFixed(2);
break;
+ case Calculations.CPM5D:
+ data = (val / 100000).toFixed(2);
Review Comment:
I think it is better as @wu-sheng suggested. Good catch.
My main reason is to allow converting it back. For instance if we have had only 1 call in a day:
How it will be displayed:
- `(69/100000).toFixed(2) == '0.00'`
- `(69/100000).toFixed(5) == '0.00069'
If the user, would like to know how many request they have had:
- `(69/100000).toFixed(2) * 1440` == 0`
- `(69/100000).toFixed(5) * 1440 == 0.9935999999999999` ~ 1 request
--
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-booster-ui] kezhenxu94 commented on a diff in pull request #239: feat: add a calculation for `cpm5d`
Posted by "kezhenxu94 (via GitHub)" <gi...@apache.org>.
kezhenxu94 commented on code in PR #239:
URL: https://github.com/apache/skywalking-booster-ui/pull/239#discussion_r1113730974
##########
src/views/dashboard/data.ts:
##########
@@ -309,7 +309,7 @@ export const CalculationOpts = [
value: "convertMilliseconds",
},
{ label: "Seconds to YYYY-MM-DD HH:mm:ss", value: "convertSeconds" },
- { label: "Precision is 2", value: "precision" },
+ { label: "CPM5D", value: "cpm5d" },
Review Comment:
Hardcoding this label to CPM5D looks unreasonable to me. This calculation type can be selected even if the chart is not `cpm`, for example in the demo video you can select `CPM5D` for the apdex charts. So I'm curious why not use the previous label like `Precision is 5 digits` or so?
--
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-booster-ui] wu-sheng commented on a diff in pull request #239: feat: add a calculation for `cpm5d`
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on code in PR #239:
URL: https://github.com/apache/skywalking-booster-ui/pull/239#discussion_r1113763846
##########
src/views/dashboard/data.ts:
##########
@@ -309,7 +309,7 @@ export const CalculationOpts = [
value: "convertMilliseconds",
},
{ label: "Seconds to YYYY-MM-DD HH:mm:ss", value: "convertSeconds" },
- { label: "Precision is 2", value: "precision" },
+ { label: "CPM5D", value: "cpm5d" },
Review Comment:
We don't have a customizable formula mechanism on UI side, so, from last time, @Fine0830 decided to hard code all possible formulas, then you see what they are today. History 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.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-booster-ui] wu-sheng commented on a diff in pull request #239: feat: add a calculation for `cpm5d`
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on code in PR #239:
URL: https://github.com/apache/skywalking-booster-ui/pull/239#discussion_r1113693642
##########
src/hooks/useMetricsProcessor.ts:
##########
@@ -398,15 +398,15 @@ export function aggregation(val: number, config: { calculation?: string }): numb
case Calculations.Apdex:
data = (val / 10000).toFixed(2);
break;
+ case Calculations.CPM5D:
+ data = (val / 100000).toFixed(2);
Review Comment:
Make sense.
--
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-booster-ui] wu-sheng commented on a diff in pull request #239: feat: add a calculation for `cpm5d`
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on code in PR #239:
URL: https://github.com/apache/skywalking-booster-ui/pull/239#discussion_r1113762854
##########
src/views/dashboard/data.ts:
##########
@@ -309,7 +309,7 @@ export const CalculationOpts = [
value: "convertMilliseconds",
},
{ label: "Seconds to YYYY-MM-DD HH:mm:ss", value: "convertSeconds" },
- { label: "Precision is 2", value: "precision" },
+ { label: "CPM5D", value: "cpm5d" },
Review Comment:
> Hardcoding this label to CPM5D looks unreasonable to me. This calculation type can be selected even if the chart is not cpm, for example in the demo video you can select CPM5D for the apdex charts. So I'm curious why not use the previous label like Precision is 5 digits or so?
This is not a label, this is a formula to format data. Check existing ones, there are things like `apdex`.
--
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-booster-ui] wu-sheng commented on a diff in pull request #239: feat: add a calculation for `cpm5d`
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on code in PR #239:
URL: https://github.com/apache/skywalking-booster-ui/pull/239#discussion_r1113763213
##########
src/views/dashboard/data.ts:
##########
@@ -309,7 +309,7 @@ export const CalculationOpts = [
value: "convertMilliseconds",
},
{ label: "Seconds to YYYY-MM-DD HH:mm:ss", value: "convertSeconds" },
- { label: "Precision is 2", value: "precision" },
+ { label: "CPM5D", value: "cpm5d" },
Review Comment:
`Precision is 5 digits` was wrong, I was misguided. `Precision is 5 digits` meant the data is having 5 digits after the dot, but actually OAP doesn't have float, only int/long.
--
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-booster-ui] wu-sheng merged pull request #239: feat: add a calculation for `cpm5d`
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng merged PR #239:
URL: https://github.com/apache/skywalking-booster-ui/pull/239
--
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