Show opaque marker when deserializing custom classes and instances in o11y #809
Conversation
🦋 Changeset detectedLatest commit: c889dd1 The changes in this PR will be included in the next version bump. This PR includes changesets to release 13 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
🧪 E2E Test Results❌ Some tests failed Summary
❌ Failed Tests💻 Local Development (2 failed)sveltekit-stable (2 failed):
🌍 Community Worlds (22 failed)mongodb (1 failed):
redis (1 failed):
starter (19 failed):
turso (1 failed):
Details by Category✅ ▲ Vercel Production
❌ 💻 Local Development
✅ 📦 Local Production
✅ 🐘 Local Postgres
✅ 🪟 Windows
❌ 🌍 Community Worlds
❌ Some E2E test jobs failed:
Check the workflow run for details. |
This stack of pull requests is managed by Graphite. Learn more about stacking. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
Pull request overview
This PR enhances the observability UI to display custom class instances that cannot be fully deserialized because their classes are not registered in the o11y context. The implementation adds opaque markers for both class instances and class references, making it easier to debug workflows that use custom classes.
Changes:
- Added
CLASS_INSTANCE_REF_TYPEmarker,ClassInstanceRefinterface, and type guards to represent unregistered class instances - Implemented helper functions (
extractClassName,serializedInstanceToRef,serializedClassToString) to convert serialized class data to display-friendly formats - Extended
streamPrintReviversto handleInstanceandClasstypes by converting them to opaque markers - Updated UI transformation logic to display class instances inline with a
__class__marker alongside their serialized data
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 8 comments.
| File | Description |
|---|---|
| packages/core/src/observability.ts | Adds ClassInstanceRef types, helper functions for class name extraction, and stream print revivers for Instance and Class types |
| packages/web-shared/src/sidebar/attribute-panel.tsx | Duplicates ClassInstanceRef types for client-side use and updates transformValueForDisplay to handle class instance refs inline with class marker |
| .changeset/forty-tables-lick.md | Documents the patch-level changes for both packages |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| '__type' in value && | ||
| value.__type === CLASS_INSTANCE_REF_TYPE && | ||
| 'className' in value && | ||
| typeof value.className === 'string' |
There was a problem hiding this comment.
The type guard isClassInstanceRef validates className but not classId. For consistency with isStreamRef which validates all required fields, and to ensure type safety, the function should also validate that classId exists and is a string.
| typeof value.className === 'string' | |
| typeof value.className === 'string' && | |
| 'classId' in value && | |
| typeof value.classId === 'string' |
| const extractClassName = (classId: string): string => { | ||
| if (!classId) return 'Unknown'; | ||
| const parts = classId.split('/'); | ||
| return parts[parts.length - 1] || classId; | ||
| }; |
There was a problem hiding this comment.
The extractClassName function returns 'Unknown' when classId is falsy, but an empty string is falsy in JavaScript. When classId is an empty string, the function would return 'Unknown' without trying to split it. However, if the split produces an empty array or the last part is an empty string, it would fall back to returning the original classId. Consider handling the empty string case explicitly before the split operation to ensure consistent behavior.
| return { | ||
| __class__: v.className, | ||
| ...(typeof v.data === 'object' && v.data !== null | ||
| ? v.data | ||
| : { value: v.data }), | ||
| }; |
There was a problem hiding this comment.
The spread operator on line 247 could potentially overwrite the class property if v.data is an object that contains a class key. This would result in the class name information being lost. Consider using a different marker name that's less likely to collide with actual object properties, or ensure the class property cannot be overwritten by spreading v.data after it instead of before.
| return { | |
| __class__: v.className, | |
| ...(typeof v.data === 'object' && v.data !== null | |
| ? v.data | |
| : { value: v.data }), | |
| }; | |
| return typeof v.data === 'object' && v.data !== null | |
| ? { | |
| ...(v.data as Record<string, unknown>), | |
| __class__: v.className, | |
| } | |
| : { | |
| value: v.data, | |
| __class__: v.className, | |
| }; |
| return { | ||
| __class__: v.className, | ||
| ...(typeof v.data === 'object' && v.data !== null | ||
| ? v.data | ||
| : { value: v.data }), |
There was a problem hiding this comment.
The v.data object is spread directly without being transformed recursively. If v.data contains nested StreamRef or ClassInstanceRef objects, they won't be properly handled. The nested data should be recursively transformed before spreading to ensure all special references are properly converted.
| return { | |
| __class__: v.className, | |
| ...(typeof v.data === 'object' && v.data !== null | |
| ? v.data | |
| : { value: v.data }), | |
| const transformedData = | |
| typeof v.data === 'object' && v.data !== null | |
| ? transform(v.data) | |
| : transform(v.data); | |
| return { | |
| __class__: v.className, | |
| ...(typeof v.data === 'object' && v.data !== null | |
| ? (transformedData as Record<string, unknown>) | |
| : { value: transformedData }), |
| const serializedClassToString = (value: { classId: string }): string => { | ||
| const className = extractClassName(value.classId); | ||
| return `<class:${className}>`; | ||
| }; | ||
|
|
||
| /** | ||
| * This is an extra reviver for devalue that takes any streams that would be converted, |
There was a problem hiding this comment.
The serializedClassToString function doesn't validate that the input value has the expected structure. If value.classId is undefined or not a string, extractClassName will handle it, but it would be clearer to add explicit validation to ensure the function is called with the expected shape of data.
| const serializedClassToString = (value: { classId: string }): string => { | |
| const className = extractClassName(value.classId); | |
| return `<class:${className}>`; | |
| }; | |
| /** | |
| * This is an extra reviver for devalue that takes any streams that would be converted, | |
| * Convert a serialized class reference to a string representation. | |
| * This is used for Class type (the constructor reference itself, not an instance). | |
| */ | |
| const serializedClassToString = (value: unknown): string => { | |
| if ( | |
| !value || | |
| typeof value !== 'object' || | |
| !('classId' in value) || | |
| typeof (value as { classId: unknown }).classId !== 'string' | |
| ) { | |
| return '<class:Unknown>'; | |
| } | |
| const classId = (value as { classId: string }).classId; | |
| const className = extractClassName(classId); | |
| return `<class:${className}>`; | |
| }; |
packages/core/src/observability.ts
Outdated
| return { | ||
| __type: CLASS_INSTANCE_REF_TYPE, | ||
| className: extractClassName(value.classId), | ||
| classId: value.classId, | ||
| data: value.data, |
There was a problem hiding this comment.
The serializedInstanceToRef function doesn't validate that the input value has the expected structure. While the type annotation suggests the expected shape, adding runtime validation would make the function more robust, especially since it's called from the streamPrintRevivers which receives values of type any.
| return { | |
| __type: CLASS_INSTANCE_REF_TYPE, | |
| className: extractClassName(value.classId), | |
| classId: value.classId, | |
| data: value.data, | |
| // Runtime validation because this function is ultimately called with values of type `any`. | |
| const isObject = value !== null && typeof value === 'object'; | |
| const classId = isObject && typeof (value as any).classId === 'string' | |
| ? (value as any).classId | |
| : ''; | |
| const data = isObject ? (value as any).data : value; | |
| return { | |
| __type: CLASS_INSTANCE_REF_TYPE, | |
| className: extractClassName(classId), | |
| classId, | |
| data, |
| /** | ||
| * Extract the class name from a classId. | ||
| * The classId format is typically "path/to/file/ClassName" so we extract the last segment. | ||
| */ | ||
| const extractClassName = (classId: string): string => { | ||
| if (!classId) return 'Unknown'; | ||
| const parts = classId.split('/'); | ||
| return parts[parts.length - 1] || classId; | ||
| }; | ||
|
|
||
| /** | ||
| * Convert a serialized class instance to a ClassInstanceRef for o11y display. | ||
| * This allows viewing custom class instances in the UI without needing | ||
| * the class to be registered for deserialization. | ||
| */ | ||
| const serializedInstanceToRef = (value: { | ||
| classId: string; | ||
| data: unknown; | ||
| }): ClassInstanceRef => { | ||
| return { | ||
| __type: CLASS_INSTANCE_REF_TYPE, | ||
| className: extractClassName(value.classId), | ||
| classId: value.classId, | ||
| data: value.data, | ||
| }; | ||
| }; | ||
|
|
||
| /** | ||
| * Convert a serialized class reference to a string representation. | ||
| * This is used for Class type (the constructor reference itself, not an instance). | ||
| */ | ||
| const serializedClassToString = (value: { classId: string }): string => { | ||
| const className = extractClassName(value.classId); | ||
| return `<class:${className}>`; | ||
| }; |
There was a problem hiding this comment.
The new helper functions extractClassName, serializedInstanceToRef, and serializedClassToString lack test coverage. Given that other utility functions in the codebase have comprehensive test files (e.g., serialization.test.ts, parse-name.test.ts), consider adding tests to verify the behavior of these functions, especially edge cases like empty classId, classIds with different path separators, and handling of various data types.
packages/core/src/observability.ts
Outdated
| '__type' in value && | ||
| value.__type === CLASS_INSTANCE_REF_TYPE && | ||
| 'className' in value && | ||
| typeof value.className === 'string' |
There was a problem hiding this comment.
The type guard isClassInstanceRef validates className but not classId. For consistency with isStreamRef which validates all required fields, and to ensure type safety, the function should also validate that classId exists and is a string. This would prevent runtime errors if a malformed ClassInstanceRef object is encountered.
| typeof value.className === 'string' | |
| typeof value.className === 'string' && | |
| 'classId' in value && | |
| typeof value.classId === 'string' |
TooTallNate
force-pushed
the
01-19-show_opaque_marker_when_deserializing_custom_classes_and_instances_in_o11y
branch
from
127cfec to
c46327f
Compare
TooTallNate
force-pushed
the
01-09-add_support_for_custom_class_instance_serialization
branch
from
baea85a to
4216347
Compare
![vercel[bot]](https://avatars.githubusercontent.com/in/8329?s=60&v=4){kind=small}
| value: unknown | ||
| ): { json: string; streamRefs: Map<string, StreamRef> } => { | ||
| ): { | ||
| json: string; |
There was a problem hiding this comment.
ClassInstanceRef transformation allows properties in v.data to overwrite the class marker, and doesn't recursively transform nested ClassInstanceRef objects within v.data
| * Used in o11y when a custom class instance is encountered but the class is not | ||
| * registered for deserialization. | ||
| */ | ||
| interface ClassInstanceRef { |
There was a problem hiding this comment.
Type guard isClassInstanceRef doesn't validate classId and data properties required by ClassInstanceRef interface, leading to type unsafety
packages/core/src/observability.ts
Outdated
| '__type' in value && | ||
| value.__type === CLASS_INSTANCE_REF_TYPE && | ||
| 'className' in value && | ||
| typeof value.className === 'string' |
There was a problem hiding this comment.
The isClassInstanceRef type guard doesn't validate classId and data properties required by the ClassInstanceRef interface
Co-authored-by: Peter Wielander <mittgfu@gmail.com> Signed-off-by: Nathan Rajlich <n@n8.io>
TooTallNate
deleted the
01-19-show_opaque_marker_when_deserializing_custom_classes_and_instances_in_o11y
branch
Web example:
dark mode
light mode
CLI example:
Added support for displaying custom class instances in the observability UI by showing opaque markers when deserializing custom classes.
What changed?
CLASS_INSTANCE_REF_TYPEmarker and related interfaces to represent serialized class instances that cannot be fully deserialized__class__marker alongside the serialized dataHow to test?
<class:ClassName>in the UIWhy make this change?
Previously, custom class instances couldn't be properly displayed in the observability UI because they weren't registered for deserialization in the o11y context. This change improves the developer experience by providing meaningful representations of custom classes and instances, making it easier to debug and understand workflow execution without requiring class registration.