search

ooni / collector

Next generation OONI collector
BSD 3-Clause "New" or "Revised" License
4 stars 0 forks source link

Mvp kiss #3

Open hellais opened 5 years ago

hellais commented 5 years ago

I think it's more reasonable to roll out the golang based collector with an incremental approach:

  1. Write a MVP Keep It Simple Silly® version of a golang collector that is identical to our current python one
  2. Write good test coverage of it
  3. Ensure we have good logging to detect defects (with sentry)
  4. Deploy it as a replacement for the current collector on one of our two collectors
  5. If all is good deprecate the legacy python based collector

Once we are confident that this doesn't create any issues and are OK with this codebase we can build upon this smaller neater golang codebase.

As part of this I written this collector in ~800 loc including tests and it only supports the most basic functionality of the current collector.

It currently has 68% code coverage, but my aim is to reach ~90% of unittest coverage and have some solid integration tests which start measurement_kit and probe-legacy to ensure that measurements are collected properly.

hellais commented 5 years ago

I am pretty happy with the level of unittest coverage I have now and it was useful to spot a couple more bugs.

Currently I don't support the legacy yaml based format, but we could possibly opt for dropping it. @bassosimone @darkk what do you think of dropping YAML format support? Do you recall what the conclusion of that discussion was?

My next step for this is writing some integration tests that run measurement_kit and probe-legacy in a docker VM to check if they work as expected.

It would be useful in the meantime to get some code review of this branch.

It's probably most agile to simply clone the repo and read the code directly (rather than looking at the diffs in here).

hellais commented 5 years ago

@bassosimone thanks a bunch for your very thorough review, it's greatly appreciated!

I believe I have addressed all your comments and here is a checklist for what I plan to do to follow up on them:

bassosimone commented 5 years ago

@hellais I think the main convo we need to have is related to an incremental strategy for increasingly enforcing more type correctness in what comes in input. We should probably have a quick chat.

bassosimone commented 5 years ago

So, recently @hellais suggested that I should probably take responsibility here and make sure this is merged, which is also incidentally aligned with my plan at https://github.com/ooni/probe/issues/863.

Let's start this process by stating that with respect to

Decide on what we should do in relation to YAML and document the decision

we WILL NOT implement YAML here.

bassosimone commented 5 years ago

Here's another thing do decide:

Check what should be done about DeprecatedUpdateReportHandler

I am inclined to drop support for this functionality. (This requires a spec bump, but whatever, it basically boils down to saying in the spec that new clients MUST NOT use that feature.)

bassosimone commented 5 years ago

I have done a first pass at understanding what we have here. It seems to me this branch describes reasonably well the point where we want to arrive. I'll do another pass tomorrow.