You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@skywalking.apache.org by "toffentoffen (via GitHub)" <gi...@apache.org> on 2023/02/20 15:43:18 UTC
[GitHub] [skywalking] toffentoffen opened a new pull request, #10420: Add cmp5d aggregation function
toffentoffen opened a new pull request, #10420:
URL: https://github.com/apache/skywalking/pull/10420
`cpm5d` function allows does not lose precision when it is downsampled to hours or days.
<!--
⚠️ 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 #10419.
- [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] wu-sheng commented on pull request #10420: Add cpm5d aggregation function
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on PR #10420:
URL: https://github.com/apache/skywalking/pull/10420#issuecomment-1459116346
About the metric name change, `service_cpm` would be about at least cause 66 places changed, including docs and UI setup.
<img width="1080" alt="image" src="https://user-images.githubusercontent.com/5441976/223594053-4e18fc23-0a6f-4bee-a155-bd7d6d8dacf4.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] wu-sheng commented on pull request #10420: Add cpm5d aggregation function
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on PR #10420:
URL: https://github.com/apache/skywalking/pull/10420#issuecomment-1447284023
I am not following, the read through GraphQL is reading values, and calculated by UI function.
So, the OAL should not be irrelevant.
--
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 #10420: Add cpm5d aggregation function
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on PR #10420:
URL: https://github.com/apache/skywalking/pull/10420#issuecomment-1459107910
@kezhenxu94 @toffentoffen Another warning, this change(update metric name) could break existing e2e.
--
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 #10420: Add cpm5d aggregation function
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on PR #10420:
URL: https://github.com/apache/skywalking/pull/10420#issuecomment-1447284938
Do you mean we need a new UI function?
--
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 #10420: Add cpm5d aggregation function
Posted by "kezhenxu94 (via GitHub)" <gi...@apache.org>.
kezhenxu94 commented on PR #10420:
URL: https://github.com/apache/skywalking/pull/10420#issuecomment-1447278964
> > But here comes the problem, if I change it to cpm5d, the list will be broken.
>
> Could you check the GraphQL to see what happens? Basically, changing the metric name(keep calculation unchanged) should not break anything. @Fine0830 Anything different here?
By "broken", I think @toffentoffen means the values are not correct, because currently a calculation "average" is already applied to each cell of the first column, if we choose to `cpm5d`, then the cpm for each service is a list (`readMetricsValues` returns a 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] wu-sheng commented on pull request #10420: Add cpm5d aggregation function
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on PR #10420:
URL: https://github.com/apache/skywalking/pull/10420#issuecomment-1459090495
BTW, if we change this, our Grafana setup has to follow.
--
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] toffentoffen commented on pull request #10420: Add cpm5d aggregation function
Posted by "toffentoffen (via GitHub)" <gi...@apache.org>.
toffentoffen commented on PR #10420:
URL: https://github.com/apache/skywalking/pull/10420#issuecomment-1458343188
@wu-sheng After some back and forth deploying the showcase, I've managed to double-check all the modified dashboard and they look correct:
- general
- mesh
- virtual_cache
- virtual_database
- virtual_mq
--
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] Fine0830 commented on pull request #10420: Add cpm5d aggregation function
Posted by "Fine0830 (via GitHub)" <gi...@apache.org>.
Fine0830 commented on PR #10420:
URL: https://github.com/apache/skywalking/pull/10420#issuecomment-1447637839
> @Fine0830 I think we need `cpm5d + AvgPreview` for table list calculation.
https://github.com/apache/skywalking/pull/10456 has merged. Please feel free to use `cpm5d + AvgPreview` calculation. @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] toffentoffen commented on pull request #10420: Add cpm5d aggregation function
Posted by "toffentoffen (via GitHub)" <gi...@apache.org>.
toffentoffen commented on PR #10420:
URL: https://github.com/apache/skywalking/pull/10420#issuecomment-1447832122
Thank you very much @Fine0830 I think with the new `cmd5dAvg` it will work.
Also, thank you @wu-sheng and @kezhenxu94 for helping here.
--
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] toffentoffen commented on pull request #10420: Add cpm5d aggregation function
Posted by "toffentoffen (via GitHub)" <gi...@apache.org>.
toffentoffen commented on PR #10420:
URL: https://github.com/apache/skywalking/pull/10420#issuecomment-1446889913
> [apache/skywalking-booster-ui#239](https://github.com/apache/skywalking-booster-ui/pull/239) has merged. Please update the relative dashboards and OAL to adopt this cpm5d function.
Let's say I have updated all the needed OAL expression to use `cpm5d()` instead of `cmp()`:
For instance `endpoint_cpm` would be:
```
# FROM
# endpoint_cpm = from(Endpoint.*).cpm();
# TO
endpoint_cpm = from(Endpoint.*).cpm5d(); # easy peasy change XD
```
So now I am trying to update the dashboards. I guess that what I need to do is to change the `calculation`(of the `metricConfig`) of any component in a dashboard that relies on any `*_cpm` metric to use the `cpm5d`, but I have some doubts about the dashboards.
Looking at the UI PR, it looks like `cmp5d` is a new calculation https://github.com/apache/skywalking-booster-ui/blob/5cc913a33233d7fb0db151af4e4e444eb3a9a24b/src/hooks/data.ts#L41.
However, now I am wondering as `cmp5d` is a Calculation, how should I change the following component:
https://github.com/apache/skywalking/blob/1335a48f1c034abc1fe24f6197ee7acfa3118bf0/oap-server/server-starter/src/main/resources/ui-initialized-templates/general/general-service.json#L715-L717
This concrete component is the following endpoints list:
The first column, is configured as "read all values of the duration"(`readMetricsValues`), and then all the values get averaged.
But here comes the problem, if I change it to `cpm5d`, the list will be broken.
The think is that we still need calculate the average, but then format it differently.
So we maybe need to have an `Average5d` calculation, and maybe other `[calculation]5d` calculations, or we have developed the solution in a way that doesn't fit correctly.
--
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] toffentoffen commented on pull request #10420: Add cpm5d aggregation function
Posted by "toffentoffen (via GitHub)" <gi...@apache.org>.
toffentoffen commented on PR #10420:
URL: https://github.com/apache/skywalking/pull/10420#issuecomment-1448242744
Okay I think I understood it now.
So in the general service table for instance:
For each service and metrics, we fetch all the values because we need them to display the spline graph as well as the table value. But the table value is computed on the client side by averaging all the fetch data points for a given service and metric.
That is the main driving force to have `cpm5dAvg` calculation functions on the UI.
--
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 #10420: Add cpm5d aggregation function
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on PR #10420:
URL: https://github.com/apache/skywalking/pull/10420#issuecomment-1448254631
> > Marc, have you deployed showcase and verify these pages?
>
> I wanted to try it out. Will do it next, will let you know @wu-sheng Need to figure out how to change the showcase project and point it to my branch.
You just need some images. Everything is here, https://github.com/apache/skywalking-showcase/blob/main/Makefile.in
--
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 #10420: Add cmp5d aggregation function
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on PR #10420:
URL: https://github.com/apache/skywalking/pull/10420#issuecomment-1437282944
@Fine0830 Could you add `Precision is 5` for this? We need to format `cpm5d` through that.
@toffentoffen We need to update CPM configuration to `CPM5D` in OAL, and settings in general and mesh dashboards, after @Fine0830 's work. Also, please fix your UT.
--
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 #10420: Add cpm5d aggregation function
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on PR #10420:
URL: https://github.com/apache/skywalking/pull/10420#issuecomment-1448121596
Marc, have you deployed showcase and verify these pages?
--
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 #10420: Add cpm5d aggregation function
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on PR #10420:
URL: https://github.com/apache/skywalking/pull/10420#issuecomment-1476205729
Close for now. As this is no update for a while, and this will break many metrics from an integration perspective. Let's see whether we really need this.
--
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 pull request #10420: Add cpm5d aggregation function
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng closed pull request #10420: Add cpm5d aggregation function
URL: https://github.com/apache/skywalking/pull/10420
--
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 #10420: Add cpm5d aggregation function
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on PR #10420:
URL: https://github.com/apache/skywalking/pull/10420#issuecomment-1459089758
> I have a doubt, now we simply replace the original `cpm` to `cpm5d`, and keep the original metrics name, when users upgrade the OAP but not UI (yet), they will find the related metrics get 100000 times by the real value, this can bring confusion. Also, this is not considered a breaking change, and the metrics data before upgrading and after upgrading will mess up, some of them are original `cpm` and some of them are `x 100000`, the final data make no sense
What do you suggest? Change the metric name in the OAL?
--
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 #10420: Add cpm5d aggregation function
Posted by "kezhenxu94 (via GitHub)" <gi...@apache.org>.
kezhenxu94 commented on PR #10420:
URL: https://github.com/apache/skywalking/pull/10420#issuecomment-1459096523
> > I have a doubt, now we simply replace the original `cpm` to `cpm5d`, and keep the original metrics name, when users upgrade the OAP but not UI (yet), they will find the related metrics get 100000 times by the real value, this can bring confusion. Also, this is not considered a breaking change, and the metrics data before upgrading and after upgrading will mess up, some of them are original `cpm` and some of them are `x 100000`, the final data make no sense
>
> What do you suggest? Change the metric name in the OAL?
I suggest so, although users might lose the original data before upgrading.
--
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] toffentoffen commented on a diff in pull request #10420: Add cpm5d aggregation function
Posted by "toffentoffen (via GitHub)" <gi...@apache.org>.
toffentoffen commented on code in PR #10420:
URL: https://github.com/apache/skywalking/pull/10420#discussion_r1112186507
##########
oap-server/server-core/src/test/java/org/apache/skywalking/oap/server/core/analysis/metrics/CPM5DecimalsMetricsTest.java:
##########
@@ -0,0 +1,114 @@
+/*
+ * 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.oap.server.core.analysis.metrics;
+
+import org.apache.skywalking.oap.server.core.remote.grpc.proto.RemoteData;
+import org.apache.skywalking.oap.server.core.storage.StorageID;
+import org.junit.jupiter.api.Assertions;
+import org.junit.jupiter.api.Test;
+
+public class CPM5DecimalsMetricsTest {
+
+ public static final long TIME_BUCKET = 2015_11_07_1130L;
Review Comment:
Does this date look familiar to anyone?
--
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 #10420: Add cpm5d aggregation function
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on PR #10420:
URL: https://github.com/apache/skywalking/pull/10420#issuecomment-1447296388
@Fine0830 I think we need `cpm5d + AvgPreview` for table list calculation.
--
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 #10420: Add cpm5d aggregation function
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on PR #10420:
URL: https://github.com/apache/skywalking/pull/10420#issuecomment-1458388570
@wankai123 Could you re-check this?
--
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 #10420: Add cpm5d aggregation function
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on code in PR #10420:
URL: https://github.com/apache/skywalking/pull/10420#discussion_r1128821193
##########
oap-server/server-starter/src/main/resources/oal/core.oal:
##########
@@ -19,15 +19,15 @@
// Service scope metrics
service_resp_time = from(Service.latency).longAvg();
service_sla = from(Service.*).percent(status == true);
-service_cpm = from(Service.*).cpm();
+service_cpm = from(Service.*).cpm5d();
Review Comment:
```suggestion
service_cpm5d = from(Service.*).cpm5d();
```
Let's follow this to update settings. @wankai123 Once this is merged, Grafana setup should be updated according on showcase.
--
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 #10420: Add cpm5d aggregation function
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on PR #10420:
URL: https://github.com/apache/skywalking/pull/10420#issuecomment-1447273072
> But here comes the problem, if I change it to cpm5d, the list will be broken.
Could you check the GraphQL to see what happens? Basically, changing the metric name(keep calculation unchanged) should not break anything.
@Fine0830 Anything different here?
--
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] toffentoffen commented on pull request #10420: Add cpm5d aggregation function
Posted by "toffentoffen (via GitHub)" <gi...@apache.org>.
toffentoffen commented on PR #10420:
URL: https://github.com/apache/skywalking/pull/10420#issuecomment-1448244834
> Marc, have you deployed showcase and verify these pages?
I wanted to try it out. Will do it next, will let you know @wu-sheng
Need to figure out how to change the showcase project and point it to my branch.
--
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 #10420: Add cpm5d aggregation function
Posted by "kezhenxu94 (via GitHub)" <gi...@apache.org>.
kezhenxu94 commented on PR #10420:
URL: https://github.com/apache/skywalking/pull/10420#issuecomment-1459088439
I have a doubt, now we simply replace the original `cpm` to `cpm5d`, and keep the original metrics name, when users upgrade the OAP but not UI (yet), they will find the related metrics get 100000 times by the real value, this can bring confusion. Also, this is not considered a breaking change, and the metrics data before upgrading and after upgrading will mess up, some of them are original `cpm` and some of them are `x 100000`, the final data make no 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] wu-sheng commented on a diff in pull request #10420: Add cpm5d aggregation function
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on code in PR #10420:
URL: https://github.com/apache/skywalking/pull/10420#discussion_r1112158932
##########
oap-server/server-core/src/main/java/org/apache/skywalking/oap/server/core/analysis/metrics/CPM5DecimalsMetrics.java:
##########
@@ -0,0 +1,31 @@
+/*
+ * 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.oap.server.core.analysis.metrics;
+
+import org.apache.skywalking.oap.server.core.analysis.metrics.annotation.MetricsFunction;
+
+@MetricsFunction(functionName = "cpm5d")
+public abstract class CPM5DecimalsMetrics extends CPMMetrics implements LongValueHolder {
+
+ @Override
+ public void calculate() {
+ this.setValue((this.getTotal() * 100000) / getDurationInMinute());
Review Comment:
```suggestion
this.setValue((this.getTotal() * 100_000) / getDurationInMinute());
```
--
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] toffentoffen commented on a diff in pull request #10420: Add cpm5d aggregation function
Posted by "toffentoffen (via GitHub)" <gi...@apache.org>.
toffentoffen commented on code in PR #10420:
URL: https://github.com/apache/skywalking/pull/10420#discussion_r1112186861
##########
oap-server/server-core/src/main/java/org/apache/skywalking/oap/server/core/analysis/metrics/CPM5DecimalsMetrics.java:
##########
@@ -0,0 +1,31 @@
+/*
+ * 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.oap.server.core.analysis.metrics;
+
+import org.apache.skywalking.oap.server.core.analysis.metrics.annotation.MetricsFunction;
+
+@MetricsFunction(functionName = "cpm5d")
+public abstract class CPM5DecimalsMetrics extends CPMMetrics implements LongValueHolder {
+
+ @Override
+ public void calculate() {
+ this.setValue((this.getTotal() * 100000) / getDurationInMinute());
Review Comment:
Applied. Thanks for the suggestion.
--
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 #10420: Add cpm5d aggregation function
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on PR #10420:
URL: https://github.com/apache/skywalking/pull/10420#issuecomment-1439300650
https://github.com/apache/skywalking-booster-ui/pull/239 has merged. Please update the relative dashboards and OAL to adopt this cpm5d function.
--
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