[WIP] (#63) Add a /stats sub-endpoint

[siruguri]

• Syntax is WIP; needs to be reviewed - currently, localhost:3000/v1/cities/stats?state=TX&fields=population,land_area&_metrics=avg,max,sum_of_squares works
• We should have an API spec test - edit: added an integration test in a second commit - shd probably be added to spec/features/api_spec.rb

[ultrasaurus]

@siruguri this is pretty sweet. Since the metrics are delivered with the results anyhow, I don't think need the /stats endpoint, especially since @yozlet added the _ prefix to all of the option params. I think it would be nice to have consistent naming -- I like the option _metrics and would use that same name for DataMagic.search option instead of add_aggregations.

let's get feedback from @shawnbot and/or @yozlet before finalizing the naming

Thank you so much!

[siruguri]

All sounds good ... will wait for more fdbk. Will rebase to @yozley's
changes shortly.

On Sun, Sep 13, 2015 at 11:44 PM, Sarah Allen [email protected]
wrote:

@siruguri https://github.com/siruguri this is pretty sweet. Since the
metrics are delivered with the results anyhow, I don't think need the
/stats endpoint, especially since @yozlet https://github.com/yozlet
added the _ prefix to all of the option params. I think it would be nice to
have consistent naming -- I like the option _metrics and would use that
same name for DataMagic.search option instead of add_aggregations.

let's get feedback from @shawnbot https://github.com/shawnbot and/or
@yozlet https://github.com/yozlet before finalizing the naming

Thank you so much!

—
Reply to this email directly or view it on GitHub
#181 (comment).

[shawnbot]

@ultrasaurus I still think that it'd be better to have a separate endpoint for stats. In our case, we need to display stats for a larger subset of the data than we're fetching to display.

For instance, we need the median value of three fields for a large subset of schools (common to all searches), but then we need to query the API for a specific subset determined by the search parameters. Having these separated into two different requests should actually improve performance because the stats queries can be cached separately.

[ultrasaurus]

For reference this is the current output of the API:

{
  "metadata": {
    "total": 100,
    "page": 0,
    "per_page": 20
  },
  "results": [
  ...
  ],
  "aggregations": {
    "population": {
      "max": 8175133,
      "avg": 600361.57
    }
  }
}

[siruguri]

I think @ultrasaurus is implying that without the _metrics option, metrics are NOT calculated. So the use case @shawnbot describes would be something like:

• /cities?state=TX&_metrics=...&_fields=my,three,fields - this would be contain the median values and would be cached separately
• /cities?state=TX - no metrics, just the cities in Texas.

Does that describe what both of you are looking to do?

[shawnbot]

@siruguri I think it does, but in that case I don't care about the results in the first request, and in the case of College Scorecard they could increase the payload size significantly. I guess that limiting the fields would help, but you'd still end up with 20 objects * 3 properties in the results array that just get ignored.

Also with College Scorecard (and, I imagine, in most apps), there are lots of fields that we need to display that we don't necessarily want stats for. In fact, I'm having a hard time imagining a scenario in which you would only want to get the fields that you've summarized. What happens, for instance, when you want the name and population of your cities, but want to summarize only the population?

I guess what I'm suggesting is that there aren't many use cases for getting results and stats in the same request, and that overloading the results endpoint with stats capabilities might lead to confusion, both in the code and among API users.

[siruguri]

I could implement it so that _metrics takes one of two values - included
and only. The latter would not generate search hits; the former would. One
endpoint; but optimal behavior ... might that be an acceptable compromise?

Tbc, I don't really care - if you give me the final word, I'll go that way
:)

On Tue, Sep 15, 2015 at 11:08 AM, Shawn Allen [email protected]
wrote:

@siruguri https://github.com/siruguri I think it does, but in that case
I don't care about the results in the first request, and in the case of
College Scorecard they could increase the payload size significantly. I
guess that limiting the fields would help, but you'd still end up with 20
objects * 3 properties in the results array that just get ignored.

Also with College Scorecard (and, I imagine, in most apps), there are lots
of fields that we need to display that we don't necessarily want stats for.
In fact, I'm having a hard time imagining a scenario in which you would
only want to get the fields that you've summarized. What happens, for
instance, when you want the name and population of your cities, but want
to summarize only the population?

I guess what I'm suggesting is that there aren't many use cases for
getting results and stats in the same request, and that overloading the
results endpoint with stats capabilities might lead to confusion, both in
the code and among API users.

—
Reply to this email directly or view it on GitHub
#181 (comment).

[yozlet]

What's this addition about? Looks like it's related to the test you added to api_spec.rb - is that right?

[yozlet]

I'm inclined towards @shawnbot's take on the API design, specifically that

1. aggregations and results should not be returned in the same request; it should be one or the other, for the reasons Shawn gave. (This also has a slight cacheability benefit)
2. Given the above, aggregations render half of the option parameters useless (along with _metrics, only _fields, _distance and _zip remain)
3. And given that aggregations also completely change the shape of the data being returned, I support the idea of appending /stats to the endpoint.

[ultrasaurus]

I like this direction! I think we're talking about an API that looks like this:

http://localhost:3000/v1/cities/stats?state=TX&_fields=population,land_area&_metrics=avg,max

that would generate results that look like this:

{
  "metadata": {
      total: 30
  },
  "metrics": {
    "population": {
      "max": 8175133,
      "avg": 600361.57
    },
    "land_area": {
      "max": 3467.78,
      "avg": 2296.44
    }
  }
}

[siruguri]

What branch shd I rebase to before continuing to work on this? dev?

[yozlet]

Yep.

[yozlet]

BTW, I recommend breaking the stats endpoint into its own controller method, and maybe add some methods down the line for it in DataMagic, QueryBuilder and ErrorChecker, rather than cramming more logic into the existing methods. Big thumbs up to any refactoring you can do here, and I'm happy to help with that too.

[siruguri]

@yozlet - QueryBuilder has a stats-specific method, but the other classes don't. I'm not super conversant with Padrino/Sinatra coding idioms, but it seems like you're suggesting something like:

- app
  - controller
    - search.rb # This would be the main endpoint's controller code
    - stats.rb # /stats

For re-factoring, maybe pull the params management out into a module in lib.
Am I getting that right?

I am not sure if the routing/controller code's ready for re-factoring already ... maybe a third endpoint will inform it a bit better? I can see cleaning up DataMagic a bit but the routing isn't super complicated yet, imo.

[yozlet]

Oh, sorry, I didn't mean to go as far as breaking everything out into separate files (though if that's where refactoring takes us, then sure). More just adding an additional get for /stats into the existing controllers.rb.

[siruguri]

Ah, ok. Got it, yes, that makes sense.

On Thu, Sep 24, 2015 at 10:56 AM, Yoz Grahame [email protected]
wrote:

Oh, sorry, I didn't mean to go as far as breaking everything out into
separate files (though if that's where refactoring takes us, then sure).
More just adding an additional get for /stats into the existing
controllers.rb.

—
Reply to this email directly or view it on GitHub
#181 (comment).

[yozlet]

Hey @siruguri, many apologies for the slowness around this PR. We're about to get funding to continue work on Open Data Maker so hopefully things should move faster soon. Many thanks for sticking with us!

[yozlet]

Given that the if clause returns a boolean anyway, you can just have it be the body of the method without the if/else.

[siruguri]

Yep, idk why my brain doesn't notice these redundant blocks.

[siruguri]

FYI, I pushed an amended commit with the edits from today's discussion, but without rebasing to dev.

[yozlet]

@siruguri Thanks for the update! However, it looks like you have a failing test - see the Travis CI failure.

[siruguri]

Sorry, I didn't realize that the test would fail on Travis - it's because
the test is too specific to the structure of the Hash response. FYI, I am
shortly going to be outside Internet contact for a few weeks, and will
probably have to get back to this in Dec. Hope that's okay.

On Mon, Nov 9, 2015 at 3:21 PM, Yoz Grahame [email protected]
wrote:

@siruguri https://github.com/siruguri Thanks for the update! However,
it looks like you have a failing test - see the Travis CI failure.

—
Reply to this email directly or view it on GitHub
#181 (comment).

[ultrasaurus]

@siruguri that is likely fine -- plenty of other things we need to do on this project, though I feel bad that you keep having to rebase! Let us know if you prefer we pick it up and integrate it. Otherwise we'll only do that if it (unexpectedly) becomes a critical feature in the interim.

[siruguri]

Ok, I'm back from vacation! :) I merged in dev and fixed the tests that failed earlier. This should be good to go now.

[yozlet]

YAY! Almost there, just one last question: What's that application.css for? (If it's just for making the front page look nicer, I suggest either (a) filing a separate PR or (b) waiting for #236)

[yozlet]

Thank you so much for sticking with us!

[yozlet]

Oh, duh, sorry, I see what happened - the application.css(.map) was generated from application.sass when you ran the server, and it stayed there and got checked in. I suggest removing it. (Maybe we should add it to .gitignore, but we can do that in a separate commit.)

[siruguri]

They were generated by rake spec, interestingly enough. I did not in fact
start the Rack server... should I just do a git rm -r public/stylesheets?

On Tue, Dec 8, 2015 at 3:32 PM, Yoz Grahame [email protected]
wrote:

Oh, duh, sorry, I see what happened - the application.css(.map) was
generated from application.sass when you ran the server, and it stayed
there and got checked in. I suggest removing it. (Maybe we should add it to
.gitignore, but we can do that in a separate commit.)

—
Reply to this email directly or view it on GitHub
#181 (comment).

[yozlet]

No, because you'll delete the application.sass from #235 and that's meant to be there. But git rm public/stylesheets/application.cs* should do the trick.

[yozlet]

@siruguri WOOHOO! Thank you so much for this fantastic contribution!

[siruguri]

:) Love the meme.

On Tue, Dec 8, 2015 at 6:04 PM, Yoz Grahame [email protected]
wrote:

Merged #181 #181.

—
Reply to this email directly or view it on GitHub
#181 (comment).

[ultrasaurus]

👍 🎉 🎈