Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Monitor Tab: Cannot choose another timeframe #898

Merged
merged 6 commits into from
Mar 1, 2022

Conversation

nofar9792
Copy link
Contributor

@nofar9792 nofar9792 commented Feb 27, 2022

Which problem is this PR solving?

Resolve (#897)

before:

Screen.Recording.2022-02-27.at.14.13.10.mov

after:

Screen.Recording.2022-02-27.at.14.14.19.mov

The issue was the calculation of the timeframe. I did in the constructor and not on every new state

Signed-off-by: nofar9792 <nofar.cohen@logz.io>
@codecov
Copy link

codecov bot commented Feb 27, 2022

Codecov Report

Merging #898 (8bbd8ae) into main (6e06ce0) will increase coverage by 0.04%.
The diff coverage is 100.00%.

❗ Current head 8bbd8ae differs from pull request most recent head 2ce04a8. Consider uploading reports for the commit 2ce04a8 to get more accurate results

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #898      +/-   ##
==========================================
+ Coverage   95.43%   95.47%   +0.04%     
==========================================
  Files         240      240              
  Lines        7471     7475       +4     
  Branches     1867     1868       +1     
==========================================
+ Hits         7130     7137       +7     
+ Misses        335      332       -3     
  Partials        6        6              
Impacted Files Coverage Δ
...r-ui/src/components/Monitor/ServicesView/index.tsx 98.87% <100.00%> (+0.05%) ⬆️
...mponents/TracePage/TraceStatistics/tableValues.tsx 98.96% <0.00%> (+1.03%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6e06ce0...2ce04a8. Read the comment docs.

albertteoh
albertteoh previously approved these changes Feb 28, 2022
Copy link
Contributor

@albertteoh albertteoh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks for fixing! Curious, is there a test we could write to prevent this from happening again?

@nofar9792
Copy link
Contributor Author

@albertteoh i added the test + small change in the implementation

@nofar9792
Copy link
Contributor Author

@yurishkuro @albertteoh I don't understand why the test coverage decreased.
My changes are not related to packages/jaeger-ui/src/components/TracePage/TraceStatistics/tableValues.tsx

@albertteoh
Copy link
Contributor

I don't understand why the test coverage decreased.

I don't understand the root cause either, but I have observed flakiness in codecov in the past with jaeger-ui PRs. The workaround was to close and re-open the PR to see if it resolves the issue, which I'll try now for you.

I'd be keen to learn if anyone else has insights on why the codecov check is failing.

@albertteoh albertteoh closed this Mar 1, 2022
@albertteoh albertteoh reopened this Mar 1, 2022
@albertteoh
Copy link
Contributor

FWIW, a link to what codecov thinks is where the coverage regression exists, which doesn't seem to be related to this PR: https://app.codecov.io/gh/jaegertracing/jaeger-ui/compare/898/changes.

Any ideas @yurishkuro?

@yurishkuro yurishkuro merged commit 161a661 into jaegertracing:main Mar 1, 2022
@nofar9792 nofar9792 deleted the fix-timeframe-bug branch March 10, 2022 22:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants