ADI-Labs / density

wireless density API
13 stars 26 forks source link

Needs to Account for Butler 301 Router Group #215

Closed alexander-yu closed 7 years ago

alexander-yu commented 7 years ago

Currently, the router group corresponding to the specific room Butler 301 hasn't been incorporated into the front page, as CUIT appears to have split the client count for Butler 301 and the rest of Butler 3 into two separate router groups. As a result, for the last year, Density has been underreporting the capacity of Butler 3 consistently by about 40-50% ever since the 301 room was renovated last October (you can sort of see it here in the Weekly Butler Device Counts plot).

I'm really seeing a few options for dealing with this:

  1. We combine the Butler 3 and Butler 301 router groups into one. This has a couple side effects:

    • We may need to recalculate the statistically expected capacity of Butler 3 because of renovations, but there may not be a need to, which is convenient (I don't remember if the number of seats in the room changed).
    • We may have to reflect this combination in the API, as the API currently displays the two separate router groups (and interestingly enough, because of the way the logic was written in the app for handling capacities, it didn't address the possibility of a group not existing in the JSON file, so it's using the Butler 3 capacity to calculate the percent fullness of Butler 301, since it was the most recent assignment to the capacity variable). If we reflect this combination in the API, we would have to account for all of the API calls, since most of them simply make a call to the database and return the query results as a JSON; we would need to either add additional logic to the Postgres queries or intercept the results before we package it as a JSON. Otherwise, we would have to deal with the side effects of option 2.
  2. We display the two as separate categories. This has a couple side effects:

    • We will need to get a statistically expected capacity for Butler 301 itself; this would fix the bug for percent fullness in the API.
    • We would need to rename Butler 3 to something a little bit more appropriate (this would involve some intercepting).
  3. We just don't deal with it; I guess this really depends on if this app is going to get redeployed to reflect changes since the original deployment (in that case, we also need to move the prediction stuff to another branch until/if it's ready for some sort of release).

Thoughts?

alanhdu commented 7 years ago

@alexander-yu Thanks for the write up!

I think Option 1 is probably the best from a user POV, but Option 2 would definitely be the easiest to do. How much work would it be to shuffle the API around for option 1? It's a little unfortunate that we don't have any API versioning built-in, and because we don't have any real analytics about who's using the API, it's unclear who'd be affected by a breaking API change. Thoughts?

That said, I'm going to punt on this for at least another week. For the last couple weeks the density data upload's broken, so fixing that's priority number one on our side.

alexander-yu commented 7 years ago

@alanhdu I think depending on our desire for efficiency, it wouldn't be too difficult; easiest way from our perspective is to add a decorator function for the API calls (and a similar function for the actual Density page) that modifies the JSON object before sending it out.

It would be a bit slow in the sense that it would be iterating through the JSON array since the data seems to be in a JSON array, rather than simply in an object, and then we would have to remove that particular element from the array. I haven't profiled the impact that would have on the API, so maybe it won't be of any significant impact.

And okay, got it; I'll probably wait on this for now.