Trace statistics table is now using antd Table component #1500

GLVSKiriti · 2023-06-14T22:39:52Z

Trace Statistics table is not using antd Table component like Trace span Table so now code is modified such that TraceStatistics table now uses antd Table component

Resolves #1181

@yurishkuro Once review this PR and let me now any changes should I do

packages/jaeger-ui/src/components/TracePage/TraceStatistics/index.tsx

yurishkuro · 2023-06-14T23:26:44Z

Did you model your code after another code in Jaeger? If so, please point to the existing code.

GLVSKiriti · 2023-06-15T07:16:33Z

Did you model your code after another code in Jaeger? If so, please point to the existing code.

Do you mean should I merge the latest changes from "main" into this branch, like by updating the branch

yashrsharma44

I have made some requests about the implementation. Can you take a look at the CONTRIBUTING.md and run the linter to make sure the variables/code are properly formatted?

packages/jaeger-ui/src/components/TracePage/TraceStatistics/index.tsx

yashrsharma44 · 2023-06-16T03:53:00Z

Did you model your code after another code in Jaeger? If so, please point to the existing code.

Do you mean should I merge the latest changes from "main" into this branch, like by updating the branch

You can link the inspiration/source of truth for the structure of the Table that you're using. I can see that we are deriving the changes, framed after the TraceSpan component.

yashrsharma44 · 2023-06-16T03:54:55Z

Thanks for raising this PR 🙂

I have some nits for raising a PR in general.

You can make the commit message a bit more descriptive, and make sure that the message depicts the changes made.

For example -
In this commit, you had introduced a sorting function - sorterFunction, so make sure the git commit message describes the changes that you have made. Currently, I can see the same commit message - Look of TraceStatistics and Tracespan tables should be similar , which makes it difficult for someone reviewing the changes to understand at a glance, what changes did you make. You can check this out for learning more about writing good commit messages :)

Check the contributing/readme for running the validation checks - e.g. linter/formatter for making sure, the code changes are compliant to the standards we have set.

…lumns Sorting Indicators added in Trace spans Table and antd Table component is used in trace statistics view Signed-off-by: GLVS Kiriti <glvskiriti2003369@gmail.com>

… in TraceStatistics table Now TraceStatistics table is working fine according to selectors in TraceStatisticsHeader and findTablesAccToSelectors function is added Signed-off-by: GLVS Kiriti <glvskiriti2003369@gmail.com>

As antd Table component is used MainTableData,DetailedTableData,HeaderTable components are not needed and sortTable file is also not needed because antd table component has sorter Signed-off-by: GLVS Kiriti <glvskiriti2003369@gmail.com>

OnCellFunction for showing the background color according to valuenameselector3 value and value of find search sorter function is modified such a way undefined type row in a subgroup of each span will be always in bottom irrespective of sorting Signed-off-by: GLVS Kiriti <glvskiriti2003369@gmail.com>

Correcting some typo errors now both TraceStatistics and TracsSpan tables look is similar and both are working fine Signed-off-by: GLVS Kiriti <glvskiriti2003369@gmail.com>

Code is now reduced and ISorterInput type is used for sorterFunction,validation checks also performed with yarn eslint Signed-off-by: GLVS Kiriti <glvskiriti2003369@gmail.com>

yashrsharma44

Thanks, the changes look much better at this point. Some small changes, and once they are addressed, it looks good to me :)

packages/jaeger-ui/src/components/TracePage/TraceStatistics/index.tsx

yashrsharma44 · 2023-06-16T14:38:35Z

You can mark the PR ready for review.

Now look of TraceStatistics and TraceSpan tables are similar Signed-off-by: GLVS Kiriti <glvskiriti2003369@gmail.com>

packages/jaeger-ui/src/components/TracePage/TraceStatistics/index.tsx

packages/jaeger-ui/src/components/TracePage/TraceStatistics/types.tsx

packages/jaeger-ui/src/components/TracePage/TraceStatistics/index.tsx

Signed-off-by: GLVS Kiriti <glvskiriti2003369@gmail.com>

yurishkuro · 2023-06-16T17:51:28Z

+202 −1,036

I love it that we'll be net-removing ~800 lines of code!

yurishkuro · 2023-06-16T18:14:07Z

A minor request: in the existing implementation sorting by column only works if you click on the up/down sort icon. Could we make sure that clicking anywhere in the column header causes the sort?

GLVSKiriti · 2023-06-16T18:22:09Z

Yes it is working like that right.clicking anywhere in the column header causes the sort

By drying the function with just one return Signed-off-by: GLVSKiriti <116095646+GLVSKiriti@users.noreply.github.com>

isDetail is a attribute of ITableSpan type which tells about sub-grouping in a table Signed-off-by: GLVSKiriti <116095646+GLVSKiriti@users.noreply.github.com>

packages/jaeger-ui/src/components/TracePage/TraceStatistics/index.tsx

Co-authored-by: Yuri Shkuro <yurishkuro@users.noreply.github.com> Signed-off-by: GLVSKiriti <116095646+GLVSKiriti@users.noreply.github.com>

type is string before now hasSubgroupValue is boolean in ITableSpan and relevant description is added in it Signed-off-by: GLVS Kiriti <glvskiriti2003369@gmail.com>

Signed-off-by: GLVS Kiriti <glvskiriti2003369@gmail.com>

packages/jaeger-ui/src/components/TracePage/TraceStatistics/types.tsx

Co-authored-by: Yash Sharma <yashrsharma44@gmail.com> Signed-off-by: GLVSKiriti <116095646+GLVSKiriti@users.noreply.github.com>

yashrsharma44

I think it looks good to me, let's wait for @yurishkuro's review 🙂

yurishkuro

lgtm

Signed-off-by: Yuri Shkuro <ysh@meta.com>

yurishkuro · 2023-06-18T19:31:04Z

not clear why unit tests are failing, did you run them locally?

Signed-off-by: GLVS Kiriti <glvskiriti2003369@gmail.com>

GLVSKiriti · 2023-06-19T05:57:29Z

not clear why unit tests are failing, did you run them locally?

Error is causing because I didn't removed the test cases for the deleted functions in index.tsx
Now I fixed them

yurishkuro · 2023-06-19T19:06:39Z

CI still failing. I recommend making sure yarn test and yarn lint are successful locally.

yarn test,yarn lint,yarn eslint,yarn prettier runned with no errors now Signed-off-by: GLVS Kiriti <glvskiriti2003369@gmail.com>

GLVSKiriti · 2023-06-19T19:15:11Z

CI still failing. I recommend making sure yarn test and yarn lint are successful locally.

Ok yarn lint also runned locally, Now every thing is working fine locally

codecov · 2023-06-19T19:32:43Z

Codecov Report

Patch coverage: 82.97% and project coverage change: +0.30 🎉

Comparison is base (beca0a8) 95.63% compared to head (e30ecc6) 95.93%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1500      +/-   ##
==========================================
+ Coverage   95.63%   95.93%   +0.30%     
==========================================
  Files         245      241       -4     
  Lines        7712     7553     -159     
  Branches     2028     1982      -46     
==========================================
- Hits         7375     7246     -129     
+ Misses        337      307      -30

Impacted Files	Coverage Δ
...i/src/components/TracePage/TraceSpanView/index.tsx	`89.39% <0.00%> (-4.26%)`	⬇️
...mponents/TracePage/TraceStatistics/tableValues.tsx	`99.33% <ø> (ø)`
...src/components/TracePage/TraceStatistics/index.tsx	`86.86% <88.37%> (+9.09%)`	⬆️
...racePage/TraceStatistics/generateDropdownValue.tsx	`95.55% <100.00%> (ø)`

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

yurishkuro · 2023-06-19T19:34:18Z

🎉 🎉 🎉 🎉 🎉 🎉

GLVSKiriti · 2023-06-19T19:43:06Z

Yayyy! Finally 😃😃🎉🎉

yashrsharma44 · 2023-06-19T20:24:59Z

❤️ 🎉

yurishkuro reviewed Jun 14, 2023

View reviewed changes

packages/jaeger-ui/src/components/TracePage/TraceStatistics/index.tsx Outdated Show resolved Hide resolved

packages/jaeger-ui/src/components/TracePage/TraceStatistics/index.tsx Outdated Show resolved Hide resolved

yashrsharma44 suggested changes Jun 16, 2023

View reviewed changes

GLVSKiriti added 6 commits June 16, 2023 14:34

In TraceSpan table sorting indicators are also added for remaining co…

29f32e6

…lumns Sorting Indicators added in Trace spans Table and antd Table component is used in trace statistics view Signed-off-by: GLVS Kiriti <glvskiriti2003369@gmail.com>

Unnecessary files are removed

9c6a827

As antd Table component is used MainTableData,DetailedTableData,HeaderTable components are not needed and sortTable file is also not needed because antd table component has sorter Signed-off-by: GLVS Kiriti <glvskiriti2003369@gmail.com>

Fixing some typo errors

7b0ea15

Correcting some typo errors now both TraceStatistics and TracsSpan tables look is similar and both are working fine Signed-off-by: GLVS Kiriti <glvskiriti2003369@gmail.com>

ISorterInput type is introduced and code is optimized

814ce87

Code is now reduced and ISorterInput type is used for sorterFunction,validation checks also performed with yarn eslint Signed-off-by: GLVS Kiriti <glvskiriti2003369@gmail.com>

yashrsharma44 suggested changes Jun 16, 2023

View reviewed changes

packages/jaeger-ui/src/components/TracePage/TraceStatistics/index.tsx Outdated Show resolved Hide resolved

packages/jaeger-ui/src/components/TracePage/TraceStatistics/index.tsx Outdated Show resolved Hide resolved

Fixing typo errors

89b552c

Now look of TraceStatistics and TraceSpan tables are similar Signed-off-by: GLVS Kiriti <glvskiriti2003369@gmail.com>

GLVSKiriti marked this pull request as ready for review June 16, 2023 15:46

GLVSKiriti requested review from everett980, rubenvp8510 and tiffon as code owners June 16, 2023 15:46

GLVSKiriti requested review from yurishkuro and yashrsharma44 June 16, 2023 16:26

yurishkuro reviewed Jun 16, 2023

View reviewed changes

Fixing typo errors and optimizing code

b82c192

Signed-off-by: GLVS Kiriti <glvskiriti2003369@gmail.com>

Merge branch 'jaegertracing:main' into traceViewsUI

b0d2553

GLVSKiriti added 2 commits June 17, 2023 00:44

Reduced the onCellFunction code

0a79a78

By drying the function with just one return Signed-off-by: GLVSKiriti <116095646+GLVSKiriti@users.noreply.github.com>

Described about isDetail

87392f3

isDetail is a attribute of ITableSpan type which tells about sub-grouping in a table Signed-off-by: GLVSKiriti <116095646+GLVSKiriti@users.noreply.github.com>

yurishkuro reviewed Jun 16, 2023

View reviewed changes

packages/jaeger-ui/src/components/TracePage/TraceStatistics/index.tsx Outdated Show resolved Hide resolved

yurishkuro reviewed Jun 17, 2023

View reviewed changes

packages/jaeger-ui/src/components/TracePage/TraceStatistics/index.tsx Outdated Show resolved Hide resolved

GLVSKiriti and others added 4 commits June 18, 2023 01:42

Editing isDetail attribute description in ITableSpan

3eeff04

Co-authored-by: Yuri Shkuro <yurishkuro@users.noreply.github.com> Signed-off-by: GLVSKiriti <116095646+GLVSKiriti@users.noreply.github.com>

Merge branch 'jaegertracing:main' into traceViewsUI

10b66c3

type is changed to hasSubgroupValue in ITableSpan type

f8d7583

type is string before now hasSubgroupValue is boolean in ITableSpan and relevant description is added in it Signed-off-by: GLVS Kiriti <glvskiriti2003369@gmail.com>

Reduced code by using columnsArray to find columns

ca881e0

Signed-off-by: GLVS Kiriti <glvskiriti2003369@gmail.com>

GLVSKiriti requested review from yurishkuro and yashrsharma44 June 18, 2023 08:11

yashrsharma44 reviewed Jun 18, 2023

View reviewed changes

packages/jaeger-ui/src/components/TracePage/TraceStatistics/types.tsx Outdated Show resolved Hide resolved

Edited description for hasSubgroupValue in ITableSpan type

52450fe

Co-authored-by: Yash Sharma <yashrsharma44@gmail.com> Signed-off-by: GLVSKiriti <116095646+GLVSKiriti@users.noreply.github.com>

yashrsharma44 approved these changes Jun 18, 2023

View reviewed changes

yurishkuro previously approved these changes Jun 18, 2023

View reviewed changes

run prettier

6a0fe26

Signed-off-by: Yuri Shkuro <ysh@meta.com>

yurishkuro dismissed their stale review via 6a0fe26 June 18, 2023 19:14

Removed testcases for functions in previous code

533474e

Signed-off-by: GLVS Kiriti <glvskiriti2003369@gmail.com>

GLVSKiriti requested review from yashrsharma44 and yurishkuro June 19, 2023 06:11

yurishkuro previously approved these changes Jun 19, 2023

View reviewed changes

yarn test,yarn lint,yarn eslint runned successfully

e30ecc6

yarn test,yarn lint,yarn eslint,yarn prettier runned with no errors now Signed-off-by: GLVS Kiriti <glvskiriti2003369@gmail.com>

GLVSKiriti dismissed yurishkuro’s stale review via e30ecc6 June 19, 2023 19:13

yurishkuro approved these changes Jun 19, 2023

View reviewed changes

yurishkuro merged commit ef41574 into jaegertracing:main Jun 19, 2023

GLVSKiriti deleted the traceViewsUI branch June 19, 2023 19:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Trace statistics table is now using antd Table component #1500

Trace statistics table is now using antd Table component #1500

GLVSKiriti commented Jun 14, 2023

yurishkuro commented Jun 14, 2023

GLVSKiriti commented Jun 15, 2023 •

edited

Loading

yashrsharma44 left a comment

yashrsharma44 commented Jun 16, 2023

yashrsharma44 commented Jun 16, 2023

yashrsharma44 left a comment

yashrsharma44 commented Jun 16, 2023

yurishkuro commented Jun 16, 2023

yurishkuro commented Jun 16, 2023

GLVSKiriti commented Jun 16, 2023 •

edited

Loading

yashrsharma44 left a comment

yurishkuro left a comment

yurishkuro commented Jun 18, 2023

GLVSKiriti commented Jun 19, 2023 •

edited

Loading

yurishkuro commented Jun 19, 2023

GLVSKiriti commented Jun 19, 2023

codecov bot commented Jun 19, 2023

yurishkuro commented Jun 19, 2023

GLVSKiriti commented Jun 19, 2023

yashrsharma44 commented Jun 19, 2023

Trace statistics table is now using antd Table component #1500

Trace statistics table is now using antd Table component #1500

Conversation

GLVSKiriti commented Jun 14, 2023

yurishkuro commented Jun 14, 2023

GLVSKiriti commented Jun 15, 2023 • edited Loading

yashrsharma44 left a comment

Choose a reason for hiding this comment

yashrsharma44 commented Jun 16, 2023

yashrsharma44 commented Jun 16, 2023

yashrsharma44 left a comment

Choose a reason for hiding this comment

yashrsharma44 commented Jun 16, 2023

yurishkuro commented Jun 16, 2023

yurishkuro commented Jun 16, 2023

GLVSKiriti commented Jun 16, 2023 • edited Loading

yashrsharma44 left a comment

Choose a reason for hiding this comment

yurishkuro left a comment

Choose a reason for hiding this comment

yurishkuro commented Jun 18, 2023

GLVSKiriti commented Jun 19, 2023 • edited Loading

yurishkuro commented Jun 19, 2023

GLVSKiriti commented Jun 19, 2023

codecov bot commented Jun 19, 2023

Codecov Report

yurishkuro commented Jun 19, 2023

GLVSKiriti commented Jun 19, 2023

yashrsharma44 commented Jun 19, 2023

GLVSKiriti commented Jun 15, 2023 •

edited

Loading

GLVSKiriti commented Jun 16, 2023 •

edited

Loading

GLVSKiriti commented Jun 19, 2023 •

edited

Loading