New generic metrics API endpoint #265
Conversation
Pull Request Test Coverage Report for Build 392855445
💛 - Coveralls |
There was a problem hiding this comment.
Well done, @macfarlandian! No major notes here, just a nitpick request.
I feel neutral about the type hints given that they have no code-time impact and are novel to the rest of the codebase, but let's see if they pick up some momentum.
This reverts commit 72f63cd.
| @@ -32,7 +32,7 @@ const isDemoMode = demoMode.isDemoMode(); | |||
| function responder(res) { | |||
| return function respond(err, data) { | |||
| if (err) { | |||
| res.send(err); | |||
| res.status(500).json({ error: err.message }); | |||
There was a problem hiding this comment.
Great catch. I've had an issue open in pulse dashboard that I haven't had time to look into yet, to figure out why fetch errors were not raising an error properly. You found it. 👍
There was a problem hiding this comment.
yeah we don't have much in the way of error handling in this app but if we did .... this would have been a problem, it just sends a blob of HTML with an error message in it or something like that
|
@jovergaag maybe eventually but it's not a big priority at the moment. If there were some major advantage to be gained like sharing type definitions across packages, etc., then I would be more keen on it, but so far nothing like that has really presented itself. With the specter of a new consolidated metrics service hanging over this I don't want to invest any more time in it than is absolutely necessary for now |
Description of the change
Extends the existing Spotlight backend to support the next-gen Spotlight client, which will request metric files arbitrarily by name rather than in pre-defined groups.
To do this I added some tests for the existing API endpoints and then refactored them to decouple the fetch action from the hard-coded metric groups so the base fetching logic can be reused. The new endpoint validates the requested files against its list of known metrics before initiating a fetch.
The new endpoint required a different caching strategy (per file instead of per group). This does technically mean the ND files would be cached twice, but since the total data volume is only on the order of a few MB I don't think that's a big concern in practice.
Also, now that there are tests for the
spotlight-apipackage, I added them to the CI workflow and coverage reports. This shouldn't shake up the overall coverage metrics very much because API test coverage is actually pretty good just from these few tests that I wrote. (For testing I factored out the Express configuration from the code that starts the server, so that I could test the Express API surface with Supertest without spawning a bunch of dangling server processes that Jest objected to. This seemed like it would be more straightforward than coming up with a server teardown process for the test environment.)You will also see some ad-hoc type hints in JSDoc comments. These are supported by, e.g., the native Typescript integration in VS Code, so I added some where it helped me reason about what I was doing, but they don't actually do anything at compile time. Nice way to bridge the gap from JS though!
Finally, note that the surface of this new endpoint somewhat resembles the designs proposed for a consolidated metrics service, though this only covers a fraction of that scope. The most significant aspects of that proposal adopted here are probably these:
Type of change
Related issues
Checklists
Development
These boxes should be checked by the submitter prior to merging:
Code review
These boxes should be checked by reviewers prior to merging: