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

Issue 526: Fix blank graph bits #581

Merged
merged 1 commit into from
Apr 16, 2024

Conversation

alecsmrekar
Copy link
Contributor

@alecsmrekar alecsmrekar commented Jan 26, 2024

#526

This is a relatively simple change that prevents the chart from rendering 0 values.

The base change is in TimeSeriesValue, where get_graph_value is converted to detect zeros, if it does, it returns a None. Converting the return value from U to Option<U> then allows us to filter out blank values when constructing chart data in add_timestamp_to_html_graph_data.

To test this, just run a few different transactions, and maybe add set_wait_time to make sure you have some seconds where nothing happened. Then open up the HTML report and looks at the pretty chart.

It's worth noting that the person who opened the original issue complained that this solution actually makes graphs looks worse, which might very well be true in some circumstances. A solution might be to adjust the chart JS a bit so that it adds dots to where data points are, so the user is not confused where requests actually happened. Example:
image

Another alternative would be to switch to a scatter plot. Which for me personally is a lot harder to read data from at a quick glance, although is solves the issue of connecting dots that are too distant.
image

@alecsmrekar alecsmrekar marked this pull request as ready for review January 26, 2024 20:12
Copy link
Member

@jeremyandrews jeremyandrews left a comment

Choose a reason for hiding this comment

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

This is a clean and simple improvement. We can explore your proposed improvements as followups if necessary. Thank you!

@jeremyandrews jeremyandrews merged commit aa77578 into tag1consulting:main Apr 16, 2024
2 checks passed
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.

2 participants