add render-time and async node error recovery pattern#802
add render-time and async node error recovery pattern#802tmarmer wants to merge 35 commits intoerror-controllerfrom
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## error-controller #802 +/- ##
====================================================
+ Coverage 86.02% 86.36% +0.33%
====================================================
Files 513 512 -1
Lines 23566 24020 +454
Branches 2703 2828 +125
====================================================
+ Hits 20272 20744 +472
+ Misses 2956 2942 -14
+ Partials 338 334 -4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Bundle ReportChanges will increase total bundle size by 125.88kB (2.17%) ⬆️. This is within the configured threshold ✅ Detailed changes
Affected Assets, Files, and Routes:view changes for bundle: plugins/check-path/coreAssets Changed:
view changes for bundle: plugins/metrics/coreAssets Changed:
view changes for bundle: react/subscribeAssets Changed:
view changes for bundle: plugins/beacon/coreAssets Changed:
view changes for bundle: plugins/markdown/coreAssets Changed:
view changes for bundle: react/playerAssets Changed:
view changes for bundle: core/playerAssets Changed:
view changes for bundle: plugins/stage-revert-data/coreAssets Changed:
view changes for bundle: plugins/reference-assets/coreAssets Changed:
view changes for bundle: plugins/async-node/coreAssets Changed:
view changes for bundle: plugins/common-expressions/coreAssets Changed:
view changes for bundle: plugins/common-types/coreAssets Changed:
|
|
/canary |
| /** Returns a function to be used as the `replacer` for JSON.stringify that tracks and ignores circular references. */ | ||
| const makeJsonStringifyReplacer = (): ReplacerFunction => { | ||
| const cache = new Set(); | ||
| return (_: string, value: any) => { | ||
| if (typeof value === "object" && value !== null) { | ||
| if (cache.has(value)) { | ||
| // Circular reference found, discard key | ||
| return "[CIRCULAR]"; | ||
| } | ||
| // Store value in our collection | ||
| cache.add(value); | ||
| } | ||
| return value; | ||
| }; | ||
| }; | ||
|
|
There was a problem hiding this comment.
Why do we need to do this? Shouldn't internal Player state be serializable?
There was a problem hiding this comment.
The issue here stems from the ResolverError adding in the failed node in its metadata. We need a better solution but this was put in place to allow the metadata to get logged without throwing errors since the node itself has circular references.
If we don't want the node logged or set in the dataController as part of captureError we either need to not include the metadata in logs/data or we need another mechanism to identify resolver nodes with to avoid sending the whole object to keep its reference.
There was a problem hiding this comment.
So this is so that the error includes the originating AST node, gotcha. I think a reference should be able to be stored fine in the Data Controller without serialization right? For logging can we just reference the asset ID?
There was a problem hiding this comment.
If the error controller was aware of the metadata being logged we could, but because any error with any metadata could pass through it, we can't identify when we're getting something like a Node or a simpler object. That and sometimes the originating Node being logged might be an async node or something else that generated the asset.
| export const ResolverStages = { | ||
| ResolveOptions: "resolve-options", | ||
| SkipResolve: "skip-resolve", | ||
| BeforeResolve: "before-resolve", | ||
| Resolve: "resolve", | ||
| AfterResolve: "after-resolve", | ||
| AfterNodeUpdate: "after-node-update", | ||
| } as const; |
There was a problem hiding this comment.
Is there a way we can automate this?
There was a problem hiding this comment.
Couldn't find a way to automatically form this object, but simplified the types here to use an enum. If you have any ideas here I'm open to try something else
…if no transition available.
core/player/src/controllers/error/utils/__tests__/isErrorWithMetadata.test.ts
Outdated
Show resolved
Hide resolved
Co-authored-by: Ketan Reddy <KetanReddy@users.noreply.github.com>
Co-authored-by: Ketan Reddy <KetanReddy@users.noreply.github.com>
…etadata.test.ts Co-authored-by: Ketan Reddy <KetanReddy@users.noreply.github.com>
… of async nodes on error
…ed across the flow instance and error controller
tmarmer
changed the title
KVSRoyal
left a comment
KVSRoyal
left a comment
There was a problem hiding this comment.
iOS stuff lgtm---can you just double-check that everything public actually needs to be?
| override val type: String | ||
| get() = ErrorTypes.RENDER | ||
| override val severity: ErrorSeverity? | ||
| get() = ErrorSeverity.ERROR | ||
| override val metadata: Map<String, Any?>? | ||
| get() = mapOf( | ||
| "assetId" to rootAsset.asset.id, | ||
| ) |
There was a problem hiding this comment.
Since these are static, just assign them:
| override val type: String | |
| get() = ErrorTypes.RENDER | |
| override val severity: ErrorSeverity? | |
| get() = ErrorSeverity.ERROR | |
| override val metadata: Map<String, Any?>? | |
| get() = mapOf( | |
| "assetId" to rootAsset.asset.id, | |
| ) | |
| override val type: String = ErrorTypes.RENDER | |
| override val severity: ErrorSeverity? = ErrorSeverity.ERROR | |
| override val metadata: Map<String, Any?> = mapOf( | |
| "assetId" to rootAsset.asset.id, | |
| ) |
| cause: Throwable? = node.getSerializable("innerErrors", ListSerializer(ThrowableSerializer()))?.first(), | ||
| override val type: String = "", | ||
| override val severity: ErrorSeverity?, | ||
| override val metadata: Map<String, Any?>?, |
There was a problem hiding this comment.
I don't think these belong on the base JSErrorException type, as that is supposed to mirror the base Error TS class. We probably want a subclass that matches the PlayerErrorMetadata one
| * Represents a Player error with metadata | ||
| */ | ||
| @Serializable(with = PlayerErrorInfo.Serializer::class) | ||
| public class PlayerErrorInfo internal constructor( |
There was a problem hiding this comment.
Should this whole class just be a PlayerException (with metadata) instance instead of a NodeWrapper? It could then just use the ThrowableSerializer instead of a whole new construct
| coroutineExceptionHandler = | ||
| config.coroutineExceptionHandler ?: CoroutineExceptionHandler { _, throwable -> | ||
| if (state !is ReleasedState) { | ||
| logger.error("[HeadlessPlayer]: Error has been found") |
There was a problem hiding this comment.
Consolidate the two logs now — just pull the other one up if you think we should log it every time regardless of whether we can forward it to captureError.
| override val type: String | ||
| get() = "TestError" | ||
| override val severity: ErrorSeverity? | ||
| get() = ErrorSeverity.ERROR | ||
| override val metadata: Map<String, Any?>? | ||
| get() = mapOf( | ||
| "testProperty" to "testValue", | ||
| ) |
| public fun setupPlayer( | ||
| plugins: List<Plugin> = this.plugins, | ||
| runtime: Runtime<*> = this.runtime, | ||
| includeThisInPlugins: Boolean = true, |
There was a problem hiding this comment.
Rather than an extra param, should we just do this automatically if this isn't contained in plugins?
| val refPlugin = ReferenceAssetsPlugin() | ||
| refPlugin.apply(runtime) | ||
| if (player is HeadlessPlayer) { | ||
| val invokable = (player as HeadlessPlayer).node.getInvokable<Unit>("registerPlugin") |
There was a problem hiding this comment.
We should just implement registerPlugin properly than hack it into a test.
| } | ||
|
|
||
| val errorMessage = assertThrows<Exception> { | ||
| val refPlugin = ReferenceAssetsPlugin() |
There was a problem hiding this comment.
Isn't this already applied on the Player instance?
6 participants
captureErrorto fail the player state if no error transition is available.PlayerViewModel.failwhich now uses the error controller.PlayerErrorMetadatainterface for errors to implement to include info required for the error controller to capture errorsChange Type (required)
Indicate the type of change your pull request is:
patchminormajorN/ADoes your PR have any documentation updates?
Release Notes
AsyncNodePlugin'sonAsyncNodeErrorhook to depend on theErrorController'sonErrorhook for finding errors.ErrorController.captureErrorand allow for error recovery on all platforms.PlayerErrorMetadatainterface across platforms and ensure serialization of errors keeps additional metadata.ErrorController.captureErrorwill prefer data from the error object when calling theonErrorhook.📦 Published PR as canary version:
0.15.1--canary.802.31569Try this version out locally by upgrading relevant packages to 0.15.1--canary.802.31569