benchttp / server

REST API for benchttp.
Other
0 stars 0 forks source link

Feat: call worker on receive report #11

Closed moreirathomas closed 2 years ago

moreirathomas commented 2 years ago

Description

Save in Firestore reports posted by the runner. Use cloud.google.com/go/firestore library.

Google Cloud uses service accounts to authenticate an application requesting access to a cloud ressource. Here we ask for read and write access to Firestore.

In a fully deployed environment, the server application would run inside Cloud Run (Cloud Engine? todo: find the correct one for our needs) and Google Cloud would under the hood find and use a default service account correctly authenticated (see Finding credentials automatically).

However in dev mode, the app runs locally and we must provide a service account credentials manually inside the GOOGLE_APPLICATION_CREDENTIALS env variable (internally read by firestore.NewClient). To do that, follow the steps from Passing credentials manually.

The service account will need read and write access to Firestore, which can be achieve by giving it the role roles/datastore.user as suggested here.

Make sure you read the security related docs!

Changes

Notes

With the flattening of the file structure, Report is define at the root inside package server, but server.Report is a super weird name. We might want to think about that (some day?).

Also, there's a lot of copy-paste regarding struct definitions, which is meh. By using gob between runner and server, the structs are coupled across repos, e.g. the fact that we need to change runner field types because Firestore does not support it (benchttp/runner#69). But runner should not really know, care or adapt to that. Subject to discussion.

~🚧 not ready for merge, as worker currently only accepts Report from Firestore, thus server uploads Report and not Output, yet.~

May close #10

GregoryAlbouy commented 2 years ago

By using gob between runner and server, the structs are coupled across repos, e.g. the fact that we need to change runner field types because Firestore does not support it (benchttp/runner#69). But runner should not really know, care or adapt to that. Subject to discussion.

Well to me there's necessarily some sort of coupling as long as they communicate directly together, should it be json or gob encoded: if a field changes it must be done both sides — but it's true gob's embarked typing adds more constraints. Let's be pragmatic: if it annoys us more than it serves us, we get rid of it. I agree runner doesn't have to know about Firestore limitations: it's indeed the job of server to adapt its input to how it stores it. But again let's be pragmatic: if the simpler way is to update the runner, and that change does not introduce explicit coupling with an implementation detail of server, then it's better to do it rather than implement a silly adapter server side to e.g. change an error to string. As long as we're not introducing logics/types that are specific to server implementation in runner, I wouldn't say runner depends on server, rather convenient input/output matching to avoid superfluous adapting complexity.

moreirathomas commented 2 years ago

I totally agree with your point on struct coupling. At the moment it is just definition that must match each other because both use the entire structure (one creates it, the other stores it as a whole).

I wanted to raise the attention around potential worse coupling in the future, because I was confronted to a slight friction when working on this feature in server.

This is fine and acceptable right now, but let's keep an eye on this !

moreirathomas commented 2 years ago

There's some de-sync between runner and server with the state of this PR.

In order to not bloat the scope here, it will be addressed in another one.