separate errors between public_api and internal#1797
Open
peaBerberian wants to merge 1 commit intodevfrom
Open
separate errors between public_api and internal#1797peaBerberian wants to merge 1 commit intodevfrom
public_api and internal#1797peaBerberian wants to merge 1 commit intodevfrom
Conversation
peaBerberian
force-pushed
the
errors-internal-api
branch
6 times, most recently
from
ce3567c to
aff16da
Compare
peaBerberian
force-pushed
the
errors-internal-api
branch
3 times, most recently
from
4d36a4d to
b9b7c04
Compare
peaBerberian
force-pushed
the
errors-internal-api
branch
from
b9b7c04 to
f73eaf3
Compare
|
✅ Automated performance checks have passed on commit DetailsPerformance tests 1st run outputNo significative change in performance for tests:
|
peaBerberian
added
the
Priority: 2 (Medium)
peaBerberian
force-pushed
the
errors-internal-api
branch
2 times, most recently
from
ea71745 to
12ef01a
Compare
|
✅ Automated performance checks have passed on commit DetailsPerformance tests 1st run outputNo significative change in performance for tests:
|
Step toward better resilience ------------------------------ We recently worked on resilience in an application, where, on various decoding issues, we would reload a content without playing the media quality that we suspect had triggered the issue. It was a big success, in the sense that decoding errors reported in production fell dramatically. Now we did that work directly in that application's code because it was less risky to do our Proof-of-concept directly in the code of the project seeing the issue than more globally in the RxPlayer. But now that it is a success, and now that we also suspect similar behavior from other players, we begin investigating doing that work inside the RxPlayer. There's multiple ways of doing it, but most of the clean solutions I'm thinking of imply `Error` instances communicating more information than what they do today. Issue with the current `Error`s ------------------------------- The more straightforward would be updating e.g. the properties from an RxPlayer's `MediaError` error in cases of decoding issues (e.g. to indicate which segment provoked the issue) but adding properties to it would mean updating our API. Updating our API comes with stability guarantees and I don't want to do that. Instead, I'll more probably on another "internal" error (errors only intended to be used inside the RxPlayer), like `CustomLoadedError` (errors linked to a `manifestLoader` / `segmentLoader`) a `SourceBufferError` (error linked to a `SourceBuffer` operation) or a `RequestError` (error linked to an HTTP request specifically - unlike the more generic `NetworkError`). But I found out that distinguing the two (internal and API errors) was unclear, they both were exported from the same path, and you could see the distinction only by reading around. What I propose here ------------------- I'm here clearly marking a separation between API errors - which now have to be imported from the `errors/public_api` path explicitly and internal errors, imported from `errors/internal`. Likewise the vague `isKnownError` has been renamed to `isApiError`. This more clearly indicates that the two are different. This opens the way to easily declare and use more external errors, which I think can be a good thing to facilitate resilience developments.
peaBerberian
force-pushed
the
errors-internal-api
branch
from
12ef01a to
5a5b19d
Compare
|
✅ Automated performance checks have passed on commit DetailsPerformance tests 1st run outputNo significative change in performance for tests:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Step toward better resilience
We recently worked on resilience in an application, where, on various decoding issues, we would reload a content without playing the media quality that we suspect had triggered the issue.
It was a big success, in the sense that decoding errors reported in production fell dramatically.
We did that work directly in that application's code because it was less risky to do our Proof-of-concept directly in the project seeing the issue than more globally in the RxPlayer.
But now that it is a success, we begin investigating doing that work inside the RxPlayer.
There's multiple ways of doing it, but most of the clean solutions I'm thinking of imply
Errorinstances communicating more information than what they do today (e.g. indicate with aBUFFER_APPEND_ERRORwhich segment provoked the issue).Issue with the current
ErrorsThe more straightforward would be updating e.g. the properties from an RxPlayer's
MediaErrorerror in cases of decoding issues but adding properties to it would mean updating our API (even if it would be undocumented, the presence of supplementary properties could led application developers to rely on them).Updating our API comes with stability guarantees and thus I don't want to do that.
Instead, I'll more probably rely on another "internal" error (errors only intended to be used inside the RxPlayer), just like we already do with
CustomLoadedError(errors linked to amanifestLoader/segmentLoader) aSourceBufferError(error linked to aSourceBufferoperation) or aRequestError(error linked to an HTTP request specifically - unlike the more genericNetworkError).But I found out that distinguing the two (internal and API errors) was unclear in the codebase, they both were exported from the same path, and you could see the distinction only by reading around.
What I propose here
I'm here clearly marking a separation between API errors - which now have to be imported from the
errors/public_apipath explicitly and internal errors, imported fromerrors/internal. Likewise the vagueisKnownErrorhas been renamed toisApiError. This more clearly indicates that the two are different.This opens the way to easily declare and use more external errors, which I think can be a good thing to facilitate resilience developments.