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

Fetching both readings for line and bar chart on meter select. #92

Closed
jameeters opened this issue Apr 29, 2017 · 5 comments
Closed

Fetching both readings for line and bar chart on meter select. #92

jameeters opened this issue Apr 29, 2017 · 5 comments
Labels
p-medium-priority t-enhancement This issues tracks a potential improvement to the software
Milestone

Comments

@jameeters
Copy link
Member

Currently when the user changes which meters are selected, we fetch readings for both line and bar charts. This will be efficient if users switch between line and bar chart for the same meters more frequently than they switch between meters within a graph type. I do not think this will be the case. Until there is some reason to believe otherwise, I think a single user action should dispatch as few API requests as possible. This was briefly discussed in re #85, and I think we came to the wrong conclusion.

@jameeters jameeters added the t-enhancement This issues tracks a potential improvement to the software label Apr 29, 2017
@sandeepacharya464
Copy link

@jameeters I feel like users who are focused on analyzing data will switch between charts quite frequently. Each graphic representation, though representing similar information, has its unique way of doing that. So, it is really helpful for analysts to switch between graphs for the same meter. But again I am not really good about the trade offs in this case. Does reducing API requests over weigh the efficiency we can deliver to small class of our users? I am not sure about that.

@johndiiorio
Copy link
Contributor

Due to the async nature of these requests, I do not believe there is any downside to requesting more data. It will not take the graph longer to display.

@jameeters
Copy link
Member Author

There is a downside to every single operation we perform.

Making the requests concurrently has absolutely no impact on the cost, only on the response time. We are still requesting data that is not for immediate use, pulling it from the database, compressing it, and caching it client side. We are doing all of this to retrieve data for which the user has expressed no desire. This will increase efficiency only if the user is almost always going to look at a line chart for meter 'a' and then a bar chart for meter 'a' with the default bar duration. (or vice-versa) Every time the user fails to do exactly this, we have made a useless request, a useless database query, and have uselessly cached the result on the user's machine.

Just to summarize:

  1. Say the user lands in line chart. If they do not switch to bar chart before changing meters, the request is wasted
  2. If the user switches to bar chart and views it in ONLY the default duration, we have gotten exactly one API request done early.
  3. If the user views only line charts for each set of meters, we are doubling the number of requests with no gain whatsoever.
  4. If the user looks at the bar chart, but is not interested in the default duration, they will need to wait on a network request for the data they are interested in anyway, and the pre-fetched data is worthless.

The changes I want will fetch useless data only when the user changes to bar chart and wants to view a different duration. When they switch chart types, they will wait longer to see their data. It avoids the useless request when the user wants only line charts. Most importantly, this avoids fetching data that will never be displayed, which I think is sloppy.

@huss huss added this to the 1.0 Release milestone Dec 10, 2020
@huss huss modified the milestones: 1.0 Release, 1.1 release Dec 26, 2021
@huss
Copy link
Member

huss commented Feb 21, 2024

I will also add that we do not get the data for 3D (due to size) until needed so this one is now separate. As we add more graphics, there is going to be a larger cost to pre-fetching.

@huss
Copy link
Member

huss commented Mar 6, 2024

Fixed by PR #1113 so closing.

@huss huss closed this as completed Mar 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p-medium-priority t-enhancement This issues tracks a potential improvement to the software
Projects
None yet
Development

No branches or pull requests

5 participants