Add GET /participants/summary

[juliangruber]

For CheckerNetwork/roadmap#154.

@bajtos currently tests are failing, since any request without from/to query parameters gets redirected to one with. This filter is meaningless in the context of this route, which currently exposes the total seen participant count. Do you have a proposal what to do about this?

[bajtos]

@bajtos currently tests are failing, since any request without from/to query parameters gets redirected to one with. This filter is meaningless in the context of this route, which currently exposes the total seen participant count. Do you have a proposal what to do about this?

Sure! Don't use the respond helper that's adding from/to redirects & caching, implement the new endpoint as a raw HTTP route.

See e.g. here:
https://github.com/filecoin-station/spark-stats/blob/5404dd1e4a300c1251005a987dea7180ef98bb3f/stats/lib/handler.js#L108-L121

A mock-up to illustrate what I mean:

// router:

 } else if (req.method === 'GET' && url === '/participants/summary') {
    await getParticipantsSummary(req, res, pgPools.stats)
 } // etc.


// implementation

// todo: add types
export const gatParticipantsSummary = async (req, res, pgPool) => {
  const { rows } = await pgPool.query(`
    SELECT COUNT(DISTINCT participant_id) FROM daily_participants
  `)

  // TODO: configure caching!

  json(res, {
    participant_count: rows[0].count
  })
}

[bajtos]

Hang on. The SQL query takes more than 400ms. I am not comfortable running that query on every request; we need to cache the result.

[juliangruber]

410b9cc

[bajtos]

Would you mind extracting these three lines into a new function? I would like to keep handlePlatformRoutes at a single level of abstraction - match request method+path and call the appropriate route handler.

[juliangruber]

I have refactored dc18234 but don't know where to put it. I also don't see a good way to refactor a generic getStatsWithCaching function. Can you please advise in more detail how you envision this?

[bajtos]

The new version looks great, thank you!

[juliangruber]

@bajtos please see my comment above: I don't know a good place to put the new function. Since it's not a data fetcher but a responder, it shouldn't be in the fetchers file

[bajtos]

@bajtos please see my comment above: I don't know a good place to put the new function. Since it's not a data fetcher but a responder, it shouldn't be in the fetchers file

I see what you mean now.

Since the new function is a route handler, I would put it at the bottom of stats/lib/platform-routes.js, below the main routing function.

We are already using that pattern in stats/lib/handler.js.

[juliangruber]

Like this? This function is so small that I think it makes the main route handler harder to read but I also value the consistency this has with other handlers

[bajtos]

Yeah, I find this new version harder to read too.

I was thinking about what I described in #248 (comment)

// router:

 } else if (req.method === 'GET' && url === '/participants/summary') {
    await respondWithParticipantsSummary(req, res, pgPools.stats)
 } // etc.

To me, I like the router to be just a router delegating to other functions, with as little configuration as necessary.

Then, the route handler function (respondWithParticipantsSummary) goes one level down the abstraction hierarchy and deals with details like TTL duration for the cache.

TBH, I think we are hitting diminishing returns here. I am fine with dc18234. I consider b1dd0f2 a bit worse, but I can live with that too.

This code is very isolated and should be super easy to change in the future if needed.

Unless you are concerned about setting up the right patterns for future contributors, in which case, I prefer the option I described at the top of this comment.

[bajtos]

👏🏻