fix(singleton): Fixed getClient, adjusted variable names, updated docstrings.#458
fix(singleton): Fixed getClient, adjusted variable names, updated docstrings.#458
Conversation
fernandocorreia-galileo
left a comment
There was a problem hiding this comment.
I'm approving, but there is a minor issue that we should fix. It's up to you if you want to do that on this PR, it's probably better to do in a follow-up PR.
Direct terminate() bypass (conceptual): If users call logger.terminate() directly instead of using reset(), lastAvailableLogger could reference a terminated logger. This violates the intended API usage pattern but is technically possible.
Here's a simple, defensive approach - modify getClient() to verify that lastAvailableLogger is still in the active loggers map:
public getClient(): GalileoLogger {
// Verify lastAvailableLogger is still active
if (this.lastAvailableLogger) {
// Check if this logger is still in our active loggers map
const isStillActive = Array.from(this.galileoLoggers.values()).includes(
this.lastAvailableLogger
);
if (isStillActive) {
return this.lastAvailableLogger;
}
// Logger was removed/terminated externally, clear the reference
this.lastAvailableLogger = null;
}
return this.getLogger();
}
Why this works:
- If someone calls reset(), the logger is removed from the map AND lastAvailableLogger is nullified (✓ already works)
- If someone calls terminate() directly, the logger stays in the map, so getClient() still returns it (
⚠️ still returns terminated logger, but that's user error) - If someone manually removes a logger from the map somehow, this check catches it
Even simpler alternative - just always fallback if there's any doubt:
public getClient(): GalileoLogger {
return this.lastAvailableLogger ?? this.getLogger();
}
And add a comment documenting that users should use reset() instead of calling terminate() directly.
My recommendation: The current code is actually fine as-is. The simple fix would be to add a JSDoc comment to getClient():
/**
* Returns the last available logger instance, or creates a new one if no logger is available.
* @deprecated Use getLogger() method instead. This method is kept for backwards compatibility.
* @important Always use reset() to terminate loggers. Calling terminate() directly may result
* in getClient() returning a terminated logger instance.
* @returns An instance of GalileoLogger
*/
This documents the expected usage pattern without adding complexity.
Opened ticket to evaluate necessary changes. Logger termination is just a call to flush, doesn't actually disable the logger in any way (including possible singleton references). |
User description
(TS SDK Parity Effort) Refactor - getClient from Galileo Singleton
[sc-48662] - https://app.shortcut.com/galileo/story/48662/ts-sdk-parity-effort-refactor-getclient-from-galileo-singleton
Description
Generated description
Below is a concise technical summary of the changes proposed in this PR:
Refactors the
GalileoSingletonto track the most recently configured logger and fixes the internal key generation logic to correctly utilize project and experiment identifiers. Enhances the logging framework's reliability by integratingAsyncLocalStoragefor context propagation and providing comprehensive test coverage for session initialization.lastAvailableLoggertracking inGalileoSingletonand refactorsgetClientto return the most recent logger instance for better state management.Modified files (1)
Latest Contributors(2)
AsyncLocalStoragecontext propagation, key precedence, and legacy method compatibility.Modified files (1)
Latest Contributors(1)
_getKeymethod to dynamically generate cache keys from context and environment variables, and adds aninitfunction for session-aware logger setup.Modified files (1)
Latest Contributors(2)