Implement a pattern for dealing with upstream APIs which are slow on the first hit; affects [endpoint] #9233
Conversation
By default error responses use the cacheLength of the service class throwing the error. This allows error to tell the handling layer the maxAge that should be set on the error badge response.
This 1. allows us to specify custom properties to pass to the exception constructor if we throw any of the standard got errors e.g: `ETIMEDOUT`, `ECONNRESET`, etc 2. uses a custom `cacheSeconds` property (if set on the exception) to set the response maxAge
|
chris48s
changed the title
core/base-service/base-json.js
Outdated
| url, | ||
| options = {}, | ||
| errorMessages = {}, | ||
| customExceptions = {}, |
There was a problem hiding this comment.
errorMessages and customExceptions do somewhat similar but importantly different things. Naming things is hard.
There was a problem hiding this comment.
ack yeah I see that being tricky down the road since both of these are (ultimately) used in the setting of error messages.
I feel like in a perfect world (one where no refactoring would be required), I'd be inclined to keep a single top level object for errors and maybe have it have two properties, one that would point to an object that's our standard http response status code to error message dictionary we have today, and the other would be the new I feel like a single top level object that had two properties, one being new dictionary that maps nodejs error codes to a desired error message.
Thinking on a bit more, I suppose having those two mappings separate as you do here is fine; I think it's just the first having squatted on the highly generic errorMessages name that feels problematic.
What if we come up with a more descriptive name for this one for now, e.g. (overly verbosely) nodeErrorCodeToShieldsExceptions and then we could potentially consider renaming errorMessages to something similarly more descriptive down the road?
There was a problem hiding this comment.
Yeah broadly agreed. Just thinking about the ergonomics of consuming this at the service layer...
Mapping HTTP error codes to error messages is really common - we do stuff like
errorMessages: { 404: 'package not found' }all over the place, so lets keep that easy to do in a convenient way.
I've designed this feature (provisionally lets call it customExceptions) to be relatively generic, but I'd expect us to actually use this quite rarely.
I think given that, I'd prefer to keep a flatter structure and avoid making errorMessages a nested object. In the vast majority of cases we would only care about one of the keys. If we can come up with 2 good names we would need to change the code in loads of places anyway, so lets make those two good names the top-level names rather than introduce another layer of nesting.
i.e: If we need to go and change a load of existing service classes, I'd rather do a big find and replace changing
errorMessages: { 404: 'package not found' }to
httpErrors: { 404: 'package not found' }than
errorMessages: { httpErrors: { 404: 'package not found' } }or whatever.
If we ignore the fact that there is loads of existing code using errorMessages and we were naming these two concepts from scratch today, what do you think would be the ideal names/API for these things? I guess the things we currently call errorMessages are really HTTP Status Codes. The thing I've called customExceptions are really System Error Codes. How about httpErrors and systemErrors ?
There was a problem hiding this comment.
How about httpErrors and systemErrors ?
Yup, love it. Definitely clears up the naming overlap and is much more succinct than the ones I threw out there
| @@ -42,6 +42,7 @@ class ShieldsRuntimeError extends Error { | |||
| if (props.underlyingError) { | |||
| this.stack = props.underlyingError.stack | |||
| } | |||
| this.cacheSeconds = props.cacheSeconds | |||
There was a problem hiding this comment.
I've done this here so any ShieldsRuntimeError can have a custom cacheSeconds but at the moment we're only using it for Inaccessible
core/base-service/got.js
Outdated
| throw new Inaccessible({ | ||
| ...customExceptions[err.code], | ||
| underlyingError: err, | ||
| }) |
There was a problem hiding this comment.
Spreading in this order means you can't manually override underlyingError. I don't think it would ever make sense to do that.
| // don't allow the user to set cacheSeconds any shorter than this._cacheLength | ||
| cacheSeconds: Math.max( | ||
| ...[this._cacheLength, cacheSeconds].filter(x => x !== undefined) | ||
| ), |
There was a problem hiding this comment.
I can't leave a line comment on it because I've not changed it in this PR, but there is an existing endpoint service test covering this.
| const cacheLength = coalesce( | ||
| serviceOverrideCacheLengthSeconds, | ||
| serviceDefaultCacheLengthSeconds, | ||
| defaultCacheLengthSeconds | ||
| ) | ||
|
|
||
| // Overrides can apply _more_ caching, but not less. Query param overriding | ||
| // can request more overriding than service override, but not less. | ||
| const candidateOverrides = [ | ||
| serviceOverrideCacheLengthSeconds, | ||
| overrideCacheLengthFromQueryParams(queryParams), |
There was a problem hiding this comment.
For the history on this, see #2755
Basically, the only thing relying on the logic as it stood here was the endpoint badge (we don't want endpoint badge users to be able to set a lower cacheSeconds value than the service default). I've moved this logic so it gets applied in endpoint.service.js so that we can now lower cacheSeconds with a custom exception property if we want. If we left this as it was, we'd only be able to raise cacheSeconds with a custom exception property (but not lower it).
chris48s
changed the title
There was a problem hiding this comment.
One thought left inline below wrt the names with one suggested change that could accompany this PR and another (potential) follow up. Don't feel terribly strongly though and wouldn't object to this moving ahead as-is with both of those items punted till later, or even if we never do either
chris48s
changed the title
chris48s
changed the title
|
If we merge anything else that uses If there are any open PRs that use |
|
Agreed 👍 |
PR badges#9233 replaced errorsMessages with httpErrors. This commit updates the new changes to stay up to date with that PR
* feat: Add author filter option for CommitActivity Add a new filter option to [GithubCommitActivity], allowing users to filter the commit activity by a specific author. To make the filter more explicit, The label display "commits by [author]" for the total amount of commits and "commit activity by [author]" for other intervals when an author filter is selected. To maintain a clear and organized code structure, The filtered author is added as an argument and not to the shield path. The request to find the number of commits by the author is made using the REST api rather then the GraphQL api to make it in 1 request rather then 2. Resolves #9215 * fix: solve eslint errors * Add tests for [GithubCommitActivity] filter by author Add tests for the new filter by author feature. * update [GithubCommitActivity] spec file for new author feat Add test for new transformAuthorFilter function of GithubCommitActivity added for the author filter feature. * Fix null string for label of GithubCommitActivity * Update GithubCommitActivity example * improve error handeling for GithubCommitActivity The author filter error handling removed was redundent as it would never execute, there is no way to seperate branch not found from repo not found. * update depricated functions PR #9233 replaced errorsMessages with httpErrors. This commit updates the new changes to stay up to date with that PR * remove test for nonexisting error this exception was removed in commit 9e358c8 and is not needed anymore * Fixed test for commit activity unexisting repo * Update example for GithubCommitActivity Picked a user with commits in the repo as an example that would work * Add test for invalid commit activity branch Add test for REST API calls in commit activity branch --------- Co-authored-by: jNullj <jNullj@users.noreply.github.com>
Closes #9174
Refs #7727
Refs #9055
This is one of those issues where I thought it probably wouldn't be that hard but it turned into a bit of a beast.
At the moment, if we throw a
ShieldsRuntimeError, the error badge uses the cacheLength of the service class throwing the error to set the response maxAge. This means if a build badge throws an error, the response has a maxAge of 30 seconds. If its a license badge then it is an hour, etc.This PR allows us to set a
cacheSecondsproperty on the exception object when we throw it which we can use to set a maxAge on certain error responses independently of the service class default.It also adds a
systemErrorsparam to the service base class fetch methods which we can use to pass extra params to the exception object when specificgoterrors are encountered.Taking all that together, this would allow a contributor to write something like
at the service layer to implement the behaviour described in #9174.
I've tried to split this into smaller commits that explain what is going on at each stage in the process.
@calebcartwright - Although it would be nice to get this merged, if you've got time to review stuff, I am more keen to get #9014 over the line tbh.