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/12/05 07:26:18 UTC
[GitHub] [skywalking] inversionhourglass opened a new issue, #10092: [Feature] Change the return value of readMetricsValue to Float(double)
inversionhourglass opened a new issue, #10092:
URL: https://github.com/apache/skywalking/issues/10092
### Search before asking
- [X] I had searched in the [issues](https://github.com/apache/skywalking/issues?q=is%3Aissue) and found no similar feature requirement.
### Description
The implementation logic of [readMetricsValue](https://github.com/apache/skywalking-query-protocol/blob/master/metrics-v2.graphqls#L146) defined in Query Protocol in oap-server is to calculate the average value within the time range and return it. Now the API returns a Long type value. For the average value, it may be more appropriate to return a Float (double) type value.
### Use case
In the process of using skywalking, I encountered a question, and I saw this question asked by others in the community. For example, the success rate of some services in the figure below is less than 100%, but no request actually fails. The reason for this problem is that there are no requests for some time periods within the currently selected time range, and the sla for these time periods without requests will be set to 0 to participate in the average calculation.
I understand that if the number of requests is too small, you should go to the service details page to view the linear chart, I also understand that if this algorithm is not used, the calculation of the failure rate will be abnormal. But I prefer that each displayed indicator can clearly show its value.
**In fact, skywalking has already supported this kind of demand very well.** For the four indicators in the figure below, the api we call is `readMetricsValues`, which queries all the indicator values in the time period. There is no metric data for unit time The zero-assignment process is performed, and the average value is finally calculated on the UI. This calculation method is very suitable for CPM indicators. If there is no request per unit time, it should be counted as zero request. For the other three indicators, we can use the `readMetricsValue` api. This interface will not fill in zeros for the time period without indicator values. The only small problem now is that the Long type value returned by this interface is not accurate enough.
### Related issues
_No response_
### Are you willing to submit a PR?
- [X] Yes I am willing to submit a PR!
### Code of Conduct
- [X] I agree to follow this project's [Code of Conduct](https://www.apache.org/foundation/policies/conduct)
--
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.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] wu-sheng commented on issue #10092: [Feature] Change the return value of readMetricsValue to Float(double)
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on issue #10092:
URL: https://github.com/apache/skywalking/issues/10092#issuecomment-1336886302
> The only small problem now is that the Long type value returned by this interface is not accurate enough.
That doesn't make sense. The precision relies on MAL, has nothing related to float or 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] wu-sheng closed issue #10092: [Feature] Change the return value of readMetricsValue to Float(double)
Posted by GitBox <gi...@apache.org>.
wu-sheng closed issue #10092: [Feature] Change the return value of readMetricsValue to Float(double)
URL: https://github.com/apache/skywalking/issues/10092
--
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] inversionhourglass commented on issue #10092: [Feature] Change the return value of readMetricsValue to Float(double)
Posted by GitBox <gi...@apache.org>.
inversionhourglass commented on issue #10092:
URL: https://github.com/apache/skywalking/issues/10092#issuecomment-1339091116
> That doesn't make sense. The precision relies on MAL, has nothing related to float or long.
If the precision is completely controlled by MAL/OAL, then it is more appropriate to display the integer milliseconds of the latency (response time) on the UI, because the unit of latency itself is milliseconds and `readMetricsValue` returns a Long type value, so the decimal place has no value when displaying in milliseconds.
Or latency increases the multiple when aggregated like sla and apdex to ensure the accuracy of average calculation, and finally divides by the multiple when visualizing.
sla and apdex is multiplied by 10000 when aggregated
https://github.com/apache/skywalking/blob/d6fa688ff8022c766cfaf3681c3b5f9113fc877f/oap-server/server-core/src/main/java/org/apache/skywalking/oap/server/core/analysis/metrics/PercentMetrics.java#L64-L66
https://github.com/apache/skywalking/blob/d6fa688ff8022c766cfaf3681c3b5f9113fc877f/oap-server/server-core/src/main/java/org/apache/skywalking/oap/server/core/analysis/metrics/ApdexMetrics.java#L92-L94
And then divided by 100 and 10000 when [visualizing](https://github.com/apache/skywalking-booster-ui/blob/1f98619a5b4ddbac4ac990dc5465b9b59bfa3349/src/hooks/useMetricsProcessor.ts#L473-L487)
```ts
case Calculations.PercentageAvg:
data = (val / 100).toFixed(2);
break;
case Calculations.Apdex:
data = (val / 10000).toFixed(2);
break;
```
The latency is collected and calculated in milliseconds of Long type, and displayed in milliseconds with two decimal places. Although there is no problem with the data, it feels a bit strange that a data without decimal places always displays these two decimal places.
https://github.com/apache/skywalking/blob/d6fa688ff8022c766cfaf3681c3b5f9113fc877f/oap-server/server-starter/src/main/resources/oal/core.oal#L20
https://github.com/apache/skywalking/blob/d6fa688ff8022c766cfaf3681c3b5f9113fc877f/oap-server/server-core/src/main/java/org/apache/skywalking/oap/server/core/analysis/metrics/LongAvgMetrics.java#L64-L66
From the previous description, it seems that this should be regarded as a visualization problem. However, if an API like `readMetricsValue` that calculates the average value can directly return a Float type value instead of rounding the average value, then the precision control can be more free.
https://github.com/apache/skywalking/blob/f1c7e939831d6a2286c0f3108cddb9241898ec1a/oap-server/server-storage-plugin/storage-elasticsearch-plugin/src/main/java/org/apache/skywalking/oap/server/storage/plugin/elasticsearch/query/MetricsQueryEsDAO.java#L95-L98
`((Number) agg.get("value")).longValue()` will round the floating-point average 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] wu-sheng commented on issue #10092: [Feature] Change the return value of readMetricsValue to Float(double)
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on issue #10092:
URL: https://github.com/apache/skywalking/issues/10092#issuecomment-1339100218
The simple answer is, ms for response time is reasonable for application service.
Don't use this to measure a method execution. I don't think a RPC latency in 0.5 or 1 ms makes any different from an APM perspective.
--
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 issue #10092: [Feature] Change the return value of readMetricsValue to Float(double)
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on issue #10092:
URL: https://github.com/apache/skywalking/issues/10092#issuecomment-1336872957
No, we are not breaking protocol.
All float number should be calculated on the visualization part. Keep the data accurate.
--
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 issue #10092: [Feature] Change the return value of readMetricsValue to Float(double)
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on issue #10092:
URL: https://github.com/apache/skywalking/issues/10092#issuecomment-1336874687
If you want to query backend oriented avg, the method has been provided.
--
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