Metrics combination API supports abandoning results#6318
Conversation
kezhenxu94
added
backend
Codecov Report
@@ Coverage Diff @@
## master #6318 +/- ##
============================================
+ Coverage 49.57% 53.02% +3.44%
- Complexity 3452 3842 +390
============================================
Files 952 1731 +779
Lines 23656 36910 +13254
Branches 2315 4080 +1765
============================================
+ Hits 11728 19572 +7844
- Misses 10915 16361 +5446
+ Partials 1013 977 -36
Continue to review full report at Codecov.
|
| if (isAbandoned) { | ||
| cachedMetrics.calculate(); | ||
| prepareRequests.add(metricsDAO.prepareBatchUpdate(model, cachedMetrics)); | ||
| nextWorker(cachedMetrics); |
There was a problem hiding this comment.
Should be add else continue here, rather than adding if (!isAbandoned) at L190?
...main/java/org/apache/skywalking/oap/server/core/analysis/worker/MetricsPersistentWorker.java
Show resolved
Hide resolved
kezhenxu94
force-pushed
the
enhance/metrics-combination
branch
from
8dbc02c to
4704c12
Compare
| * Merge the given metrics instance, these two must be the same metrics type. | ||
| * | ||
| * @param metrics to be merged | ||
| * @return {@code true} if the combined metrics should be continuously processed, {@code false} it should be abandoned. |
There was a problem hiding this comment.
| * @return {@code true} if the combined metrics should be continuously processed, {@code false} it should be abandoned. | |
| * @return {@code true} if the combined metrics should be continuously processed. {@code false} means it should be abandoned, and the implementation needs to keep the data unaltered in this case. |
I add a little more to the comments, from my code reading, if the change has been made, the data will still flush into the database eventually.
There was a problem hiding this comment.
Also, I feel the data in the cache has refreshed repeatedly into database even without change, could you recheck about this too? If so, we need to fix that in another PR, which should be able to reduce the database load.
There was a problem hiding this comment.
Also, I feel the data in the cache has refreshed repeatedly into database even without change, could you recheck about this too? If so, we need to fix that in another PR, which should be able to reduce the database load.
@kezhenxu94 ⬆️ What do you think about this?
There was a problem hiding this comment.
Because only limited Event data falls into this case, this may be not a big problem for now. Anyway, let me check and think about how to deal with this
There was a problem hiding this comment.
I don't mean the change you made, I mean for all metrics, if the metrics exist in the cache, even no update, they are keeping being updated periodly.
There was a problem hiding this comment.
I don't mean the change you made, I mean for all metrics, if the metrics exist in the cache, even no update, they are keeping being updated periodly.
OK I mistakenly thought you only referred to the "abandoned" metrics, it makes sense to me now, I'll check
…/server/core/analysis/metrics/Metrics.java Co-authored-by: 吴晟 Wu Sheng <wu.sheng@foxmail.com>
2 participants
CHANGESlog.