Add time based segmenting

[mxstbr]

Closes #3, cc @sean-roberts

[mxstbr]

Dang this bumps the size above 100 LoC, it's now 102 😢 need to fix that...

[codecov-io]

Current coverage is 93.75% (diff: 100%)

Merging #4 into master will increase coverage by 0.59%

@@             master         #4   diff @@
==========================================
  Files             4          4          
  Lines            73         80     +7   
  Methods          13         15     +2   
  Messages          0          0          
  Branches         12         17     +5   
==========================================
+ Hits             68         75     +7   
  Misses            5          5          
  Partials          0          0          

Powered by Codecov. Last update e899065...e3c55ba

[mxstbr]

We're also missing the ability to do before and after at the same time! Need to fit that in somehow...

[sean-roberts]

I think Before/After are good too. Go with that.

The code looks fine as it is.

I do think, however, the API could be better. (going with before/after) I think that ?before and ?after should be supported for both endpoints types (all/count). With the understood downside that it's not as performant of a query. But I think the primary reason most people would use get time-segments is to get the counts between those times. So providing it in the GET count-based requests requires no extra work to get the details. The usefulness of ?all is primarily targeted at some form of more advanced interface like a UI or CLI to handle calculations for the user.

I also think that you should support the combination of ?after={time}&before={time} in which should

• return nothing if before < after
• return a filter of the metrics between the two bounds. return m.time < before && m.time > after;

[mxstbr]

Added support for specifying both before and after. Not sure what you mean by this being support for both endpoints? What's the use of supporting /path?before=123?

[mxstbr]

Should we return an error if before and after are the wrong way around, i.e. if before < after, instead of just returning nothing?

[sean-roberts]

I think an error would be fine for that.

So, I don't actually know why anyone would use ?before by itself. But it comes pretty cheap to support if they did.

[mxstbr]

It's already supported 😉 will change to an error!

[luisrudge]

I'm not sure you should filter in the API itself. You should filter in the db.js file, so we can implement another db with filtering later.

[sean-roberts]

I tend to agree @luisrudge

[mxstbr]

I actually agree @luisrudge, but that's out of scope for this PR I think. If we do that, we also have to move the filtering of the keys to db.js and fix the whole testing setup.

Let's ship this for now and rework in another PR when we introduce database adapters?

[sean-roberts]

Is the PR going to support the before & after on GET?

[mxstbr]

Is the PR going to support the before & after on GET?

It doesn't currently, what's the use? GET /id will always just return a single view...

[sean-roberts]

https://github.com/mxstbr/micro-analytics/pull/4#issuecomment-271626714

[mxstbr]

Yeah I didn't quite get that 😉 would you mind elaborating? Where do you want to see it supported?

[sean-roberts]

ha, I'll try again

So my thought process is, what is the benefit from using ?all? It's to get the detailed timing of events. But I don't think people will commonly use it for the detail that it provides. I think people would use it to figure out the total views in a time segments. That is because, as time goes on, GET + ?inc=false becomes less useful. So people will fall back to having to do ?all and then have to add logic to do the counts of views.

With that, I think if GET + ?inc=false is used the user should be able to get the views count AND have the option to submit before and after time segments. It would just make the experience of that endpoint remain useful as time goes on.

So the result of this work would be, you can use ?before={time}, ?after={time}, or ?before={time}&after={time} on the ?all endpoint and the GET + ?inc=false endpoint. Allowing for quick aggregated stats on the GET + ?inc=false that can be time scoped. And more advanced results for ?all that can also be scoped.

Does that resonate better?

[mxstbr]

Good call, I totally forgot about ?inc=false! I think we might need to do this as part of #8, as doing it in core now and then moving that over will be a lot harder...

[mxstbr]

Closing in favor of landing #14!