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

[AMORO-1771] Add two line charts to the Table Transactions page, showing the count of table records and the count of table files. #1859

Merged
merged 5 commits into from
Aug 24, 2023

Conversation

minteliuwm
Copy link
Contributor

Why are the changes needed?

resolve #1771

Brief change log

  • Add two line charts to the Table Transactions page, showing the count of table records and the count of table files.

How was this patch tested?

  • Add some test cases that check the changes thoroughly including negative and positive cases if possible

  • Add screenshots for manual tests if appropriate

  • Run test locally before making a pull request

Documentation

  • Does this pull request introduce a new feature? (no)
  • If yes, how is the feature documented? (not applicable)

…ing the count of table records and the count of table files.
@github-actions github-actions bot added the module:ams-dashboard Ams dashboard module label Aug 20, 2023
@codecov
Copy link

codecov bot commented Aug 20, 2023

Codecov Report

Patch coverage has no change and project coverage change: -0.13% ⚠️

Comparison is base (389f1a4) 51.02% compared to head (44eeb8b) 50.90%.
Report is 2 commits behind head on master.

Additional details and impacted files
@@             Coverage Diff              @@
##             master    #1859      +/-   ##
============================================
- Coverage     51.02%   50.90%   -0.13%     
- Complexity     3795     3806      +11     
============================================
  Files           467      477      +10     
  Lines         25496    25631     +135     
  Branches       2599     2608       +9     
============================================
+ Hits          13010    13047      +37     
- Misses        11321    11418      +97     
- Partials       1165     1166       +1     
Flag Coverage Δ
core 51.36% <ø> (-0.16%) ⬇️
trino 48.67% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

see 21 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@shidayang
Copy link
Contributor

Thanks for your contributions, Ccould you provide a screenshot of the modified style?

@minteliuwm
Copy link
Contributor Author

Thanks for your contributions, Ccould you provide a screenshot of the modified style?

Of course, I posted the screenshot in the comments, it's implemented as the designed page, a little different is the x-axis shows the full date and time, if you think something is not implemented correctly please feel free to suggest it!
Snipaste_2023-08-21_22-14-30

@wangtaohz
Copy link
Contributor

If I understand correctly, each point in the graph corresponds to a Transaction, right? @minteliuwm

I'm concerned that displaying these points uniformly in the x direction may cause distortion, because the time intervals between Transactions are not fixed.

@minteliuwm
Copy link
Contributor Author

Transaction

@wangtaohz

Yes, currently each point on the graph represents the commit time of a transaction.
I assume you want to use the x-axis as a timeline over a period of time and then show the corresponding data at specific points in time?
I'm not quite sure of the business logic here, if you want to see where the data is going on a time trend through this chart, I think your idea is fine, but there are just two areas that might need a bit of attention:

  1. Currently our data is in milliseconds, do we need to be accurate to milliseconds here? If it is not accurate to milliseconds, there may be multiple data within the same time, is it necessary to consolidate the data in this case?
  2. The time range of the return may be relatively large, resulting in some of the close time points being crowded on the graph, is this acceptable behaviour?

@wangtaohz
Copy link
Contributor

Thank you for your prompt reply!

As for the two questions you mentioned, for the first one, I think accuracy to the second is sufficient because based on my experience, it is unlikely that multiple Transactions would be committed within one second.

As for the second question, I think it is acceptable.

@hameizi Could you provide some suggestions on how these two issues were considered during design?

@hameizi
Copy link
Contributor

hameizi commented Aug 23, 2023

Thank you for your prompt reply!

As for the two questions you mentioned, for the first one, I think accuracy to the second is sufficient because based on my experience, it is unlikely that multiple Transactions would be committed within one second.

As for the second question, I think it is acceptable.

@hameizi Could you provide some suggestions on how these two issues were considered during design?

We have discussed the display logic for this section before, and there is a theoretical possibility of submitting a large number of snapshots in a short time. Therefore, the current display logic is: only the snapshot of the current page is displayed, and the distance between the snapshots in the chart is equal.

@wangtaohz
Copy link
Contributor

We have discussed the display logic for this section before, and there is a theoretical possibility of submitting a large number of snapshots in a short time. Therefore, the current display logic is: only the snapshot of the current page is displayed, and the distance between the snapshots in the chart is equal.

Make sense to me. 👍

In the scenario where only Transactions from the current page are displayed, I agree with the current implementation.

Copy link
Contributor

@zhoujinsong zhoujinsong left a comment

Choose a reason for hiding this comment

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

Validate in my local environment.

@zhoujinsong zhoujinsong merged commit c936884 into apache:master Aug 24, 2023
@wangtaohz wangtaohz changed the title [WIP] [AMORO-1771] Add two line charts to the Table Transactions page, showing the count of table records and the count of table files. [AMORO-1771] Add two line charts to the Table Transactions page, showing the count of table records and the count of table files. Sep 20, 2023
ShawHee pushed a commit to ShawHee/arctic that referenced this pull request Dec 29, 2023
…, showing the count of table records and the count of table files. (apache#1859)

* [AMORO-1771] Add two line charts to the Table Transactions page, showing the count of table records and the count of table files.

* fix: x-axis data should not be duplicated

* style: optimize line chart style

* build: build frontend project

* fix: The x-axis shows specific data points
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
module:ams-dashboard Ams dashboard module
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Subtask]: Implement displaying metrics on the front end of AMS
5 participants