feat(logger): Improved batch featured for logger, added structure for stream mode support.#456
feat(logger): Improved batch featured for logger, added structure for stream mode support.#456
Conversation
a1accf4 to
9160788
Compare
fernandocorreia-galileo
left a comment
There was a problem hiding this comment.
Please look at test coverage. This is what Claude reported:
Missing Tests for New Features
- No tests for streaming mode functionality
- No tests for TaskHandler class
- No tests for retry logic
- No tests for EventSerializer edge cases
- Tests only cover batch mode scenarios (tests/utils/galileo-logger.test.ts:154-256)
Also see the other comments below.
src/utils/retry-utils.ts
Outdated
| throw error; | ||
| } | ||
| // Non-retryable exception - log and return null | ||
| console.error(`Unrecoverable failure or unrecognized error: ${error}`); |
There was a problem hiding this comment.
I think that a retry function like this one must either return a union type of T | Error, or at least have a hook for error handling like onExhausted?: (error: Error, attemptsMade: number) => void;. Logging to console and silently failing is not a solid foundation.
There was a problem hiding this comment.
We should discuss the design choice of logging errors for streaming mode vs other options.
| return false; | ||
| } | ||
| return ( | ||
| statusCode === 404 || |
There was a problem hiding this comment.
Retrying on 404 by default does not seem to make sense.
What's the use case that you have in mind for this? Is this for retrying until a resource is ingested?
There was a problem hiding this comment.
The issue is that this should be a generic function that can be used for any operation, not just traces.
In a future iteration I recommend refactoring this to use a more restrictive set of retriable errors, and allow specific use cases to add new ones.
Added missing test cases. These will be reviewed with following tasks that continue work on streaming mode. |
| return false; | ||
| } | ||
| return ( | ||
| statusCode === 404 || |
There was a problem hiding this comment.
The issue is that this should be a generic function that can be used for any operation, not just traces.
In a future iteration I recommend refactoring this to use a more restrictive set of retriable errors, and allow specific use cases to add new ones.
… stream mode support.
…ned/null, handleTaskFailure added to support parent tasks failures, removed underscore from private methods.
… error message, reusing repeated code into ensureClientInitialized().
68a31a8 to
250f2a5
Compare
User description
(TS SDK Parity Effort) Refactor - Galileo Logger
[sc-44815] - https://app.shortcut.com/galileo/story/44815/ts-sdk-parity-effort-refactor-galileo-logger-logger-parity
Description
Obs.: Streaming will still go through further improvement in future tasks.
Generated description
Below is a concise technical summary of the changes proposed in this PR:
Refactor the
GalileoLoggerto support streaming mode and improve batch functionality as part of the TypeScript SDK parity effort. Introduce aTaskHandlerfor managing asynchronous API requests with dependency tracking and implement a robustEventSerializerfor complex object serialization.EventSerializerfor handling complex JavaScript objects, refiningtoJSONmethods for various span and step types, and updating type definitions to use camelCase and new JSON-serializable types.Modified files (14)
Latest Contributors(2)
GalileoLogger, including theTaskHandlerfor managing concurrent asynchronous tasks,AsyncLocalStoragefor context propagation, and updated API client methods for trace and span ingestion and updates. This also includes new configuration options forprojectId,logStreamId,traceId, andspanId.Modified files (12)
Latest Contributors(2)