micro-analytics / micro-analytics-cli

Public analytics as a Node.js microservice. No sysadmin experience required! 📈
MIT License
734 stars 39 forks source link

Adapter for mongodb #58

Open esakkiraj opened 7 years ago

esakkiraj commented 7 years ago

Hi

I am half way in writing a adapter for mongodb. Implementation is based on the given spec. Yet to published in npm.

Repo: https://github.com/esakkiraj/adapter-mongodb

relekang commented 7 years ago

Cool, looks good 🎉

mxstbr commented 7 years ago

That's awesome, can't wait to see this finished!

esakkiraj commented 7 years ago

Hi @relekang @mxstbr

Have almost done the implementation ( only two test's are failing, looking at it ). Meanwhile i thought there is mismatch in the specification and test for adapters for filter option.

Filter options before & after will be part of filter key in the options object. But in the adapter's test suite before and after key are passed in options object skipping the filter key.

Note: Currently i have managed to run the test by changing the implementation.

mxstbr commented 7 years ago

@relekang any comments? I'm totally not in the context here, haven't looked at that API in a while.

relekang commented 7 years ago

Looks like the docs and implementation is the same. The implementation sends this as options.

{
  pathname: pathname,
  before: parseInt(query.before, 10),
  after: parseInt(query.after, 10),
}
esakkiraj commented 7 years ago

Hi @relekang

As per the spec ( https://github.com/micro-analytics/micro-analytics-cli/blob/master/writing-adapters.md#options) filter is passed like this

await adapter.get('/hello', { filter: { after: 1200, before: 1234 }}) // -> { views: 20 }

but in the adapters-test suite the call is like

expect(await adapter.get('/a-key', { before: 1490623475640 })).toEqual({ views: [{ time: 1490623474639 }], });

I expected it to be

expect(await adapter.get('/a-key', { filter: { before: 1490623475640 } })).toEqual({ views: [{ time: 1490623474639 }], });

Don't know if i am missing something here.

relekang commented 7 years ago

Yeah, that means that the spec differs from the implementation of micro-analytics-cli. If you follow the spec it will not work as expected, but if you follow the tests it will.

Not sure what we should do from here. If we should change the spec of change the implementation?

mxstbr commented 7 years ago

I think let's change the Spec, since it's easier to do? I don't think there's big of a difference between those two APIs...

relekang commented 7 years ago

The spec is updated now.