[spm] GetLatencies Implementation #7290
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: pipiland2612 <nguyen.t.dang.minh@gmail.com>
Signed-off-by: pipiland2612 <nguyen.t.dang.minh@gmail.com>
Signed-off-by: pipiland2612 <nguyen.t.dang.minh@gmail.com>
Signed-off-by: pipiland2612 <nguyen.t.dang.minh@gmail.com>
Signed-off-by: pipiland2612 <nguyen.t.dang.minh@gmail.com>
Signed-off-by: pipiland2612 <nguyen.t.dang.minh@gmail.com>
Signed-off-by: pipiland2612 <nguyen.t.dang.minh@gmail.com>
Signed-off-by: pipiland2612 <nguyen.t.dang.minh@gmail.com>
internal/storage/metricstore/elasticsearch/testdata/output_latencies.json
Outdated
Show resolved
Hide resolved
|
During testing, I found a bug in JaegerUI when calculating the mean of getLatencies or getCallRate. That is, the denominator when calculating also includes the count for NaN values, thus undervalue the mean. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #7290 +/- ##
==========================================
- Coverage 96.16% 96.15% -0.01%
==========================================
Files 374 374
Lines 22696 22750 +54
==========================================
+ Hits 21825 21876 +51
- Misses 657 659 +2
- Partials 214 215 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Signed-off-by: pipiland2612 <nguyen.t.dang.minh@gmail.com>
a21573d to
4d3e940
Compare
|
closes and reopens pr to rerun codecov |
4 tasks
yurishkuro
reviewed
Jul 2, 2025
Signed-off-by: pipiland2612 <nguyen.t.dang.minh@gmail.com>
e46cfd3 to
217f1ae
Compare
Signed-off-by: pipiland2612 <nguyen.t.dang.minh@gmail.com>
Signed-off-by: pipiland2612 <nguyen.t.dang.minh@gmail.com>
Signed-off-by: pipiland2612 <nguyen.t.dang.minh@gmail.com>
yurishkuro
reviewed
Jul 4, 2025
yurishkuro
reviewed
Jul 4, 2025
|
Alright, that makes sense to first refactor changes related to GetCallRate then we can continue working with this PR. |
|
Refactoring at #7295 |
Signed-off-by: pipiland2612 <nguyen.t.dang.minh@gmail.com>
Signed-off-by: pipiland2612 <nguyen.t.dang.minh@gmail.com>
|
Hi @yurishkuro, I have merge with main and resolves conflict. I have also remove the indirection from GetLatencies. It should be good now! Thanks for your feedbacks. |
yurishkuro
approved these changes
Jul 6, 2025
There was a problem hiding this comment.
potential future refactoring suggestions:
- you reuse
dateHistAggNameconstant in two files, that's a sign of abstraction leak - it might be useful to create another file
query.gowhich has a struct that deals with constructing queries and retrieving the results. It would know about things likedateHistAggName, while neither Reader nor Transformer would have to know, they would just delegate to that Query struct. - The separation of responsibilities between Reader and Transformer is unclear. Retrieving data from ES results should be part of the Query. Transformer then transforms those results. But you have a lot of business logic in the Reader which also does transformation. Please try to define (write struct comments) very clear definition of what each struct is responsible for.
|
Hi @yurishkuro, really thank you for your time for my pull request. For the refactoring, I will definitely invest time in it and soon make a follow up pr soon! |
auto-merge was automatically disabled
July 6, 2025 16:40
Head branch was pushed to by a user without write access
Signed-off-by: pipiland2612 <nguyen.t.dang.minh@gmail.com>
c6ce182 to
c85a7f5
Compare
|
Hi @yurishkuro, I have added test to increse coverage to over 95%. Can you merge it again? Thank you for your time! |
yurishkuro
approved these changes
Jul 6, 2025
4 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Which problem is this PR solving?
Description of the changes
How was this change tested?
Proof of this new feature
Checklist
jaeger:make lint testjaeger-ui:npm run lintandnpm run test