rit-sse / old-api

The unified SSE API.
MIT License
1 stars 5 forks source link

Statistics Root Endpoint Functionality #30

Closed craigcabrey closed 9 years ago

craigcabrey commented 9 years ago
bbesmanoff commented 9 years ago

What kind of information would each endpoint provide? What is the benefit of having this over app/client-based statistics?

For arguments sake, take average headcount over the past week. It doesn't feel like the api should be the one to handle that kind of information, as it is specific to the headcounting app. Asking for the average members over the past week doesn't make sense.

craigcabrey commented 9 years ago

Where else would you have this go? Any other implementation is first going to have to ask for all the data and then do processing on it. All this endpoint does is generate data from data it stores. Nothing is modified or otherwise touched.

It's harmless.

bbesmanoff commented 9 years ago

All this endpoint does is generate data from data it stores. Nothing is modified or otherwise touched.

I understand this.

Any other implementation is first going to have to ask for all the data and then do processing on it.

Right. It makes sense to me to look in a repo called headcount and find out what/how statistics are generated for headcounting. It doesn't make sense to look in the repo called api, for a file called statistics.php to a method called getHeadcountStatistics() to see how statistic calculations are done.

craigcabrey commented 9 years ago

I wholeheartedly disagree. You are proposing to fragment this unifying API for the sake of separating out three endpoints, which also has the effect of significantly increasing chattiness with no additional benefits.

It seems many RESTful APIs include statistics, including GitHub itself. I'm not seeing legitimate reasoning here beyond moving it for the sake of moving. That said, you are absolutely free to build up a separate stats app yourself and have it talk to this API.

bbesmanoff commented 9 years ago

separating out three endpoints

They already exist. GET /headcounts, GET /members, GET /memberships. What I don't like as much is that we are now adding three additional endpoints that perform calculations specific to an application. What if we want to now do statistics on quotes or events? Time to add getQuoteStats() and getEventStats() and redeploy the api instead of the app that needs the statistics.

Github's statistics API are not a one-to-one relation. Github stats are expensive to calculate. I would want them to handle that, as my machine is nowhere near as powerful as theirs for calculation. We are doing simple stuff - averages, maybe standard deviation, etc.

I'm not promoting fragmenting the API back into each individual app. I am proposing that apps should be smart enough to do averages for themselves.

kocsenc commented 9 years ago

@bbesmanoff, the architecture is :surfer: enough that adding getQuoteStats() or getXStats() and redeploying is super easy.

:hamburger:

djrenren commented 9 years ago

@craigcabrey

Just seeing this now. What kind of stats do we want and why are they part of the API?

And where are we consuming this?

kristenmills commented 9 years ago

Also just seeing this:

  1. I don't believe this is necessary. I can't think of any statistics that would warrant endpoints and I will stand by this point unless someone can provide me with an example. We're supporting complex queries on our endpoints. I'm perfectly capable at looking at the total results of that. It's not going to be noticeably faster to get the count or an average from a dedicated statistics endpoint. So TL;DR I don't see the point.
  2. If this were to exists, @bbesmanoff is right about. If we want statistics that are specific to members, it should be under GET /members/statistics and not GET /statistics/members. All your statistics are specific to an endpoint, why fragment the data?