refactor(http,llm): Using new Logger module from core#287
Conversation
bnchrch
left a comment
There was a problem hiding this comment.
Main thing: the refactor dropped the log assertions instead of pointing them at Logger, so a few "we warn on X" behaviors are now untested (inline). Couple of nits too.
| @@ -78,7 +76,6 @@ describe( 'calculateLLMCallCost', () => { | |||
| } ); | |||
|
|
|||
| expect( result ).toBeNull(); | |||
There was a problem hiding this comment.
🧪 The swap dropped the log checks instead of moving them to Logger, so we no longer test that these warnings fire:
Failed to fetch models pricingandMissing cost reference for model(this file)- the error log in the catch (this file)
- the no-cache / stale-cache warnings in
fetch_models_pricing.spec.js - the snake_case warning in
validations.spec.js - the warn / no-warn checks in
http/cost.spec.ts
In tests Logger.* falls back to console.log, so a console.log spy works, or mock @outputai/core's Logger. If we've decided log lines aren't worth testing that's fine, I'd just want it on purpose, not a side effect of the swap.
There was a problem hiding this comment.
I changed on purpose, as I dont think those are worth checking...
| @@ -1,5 +1,6 @@ | |||
| import { ValidationError, z } from '@outputai/core'; | |||
| import { attributesSchema } from './block_options.js'; | |||
| import { Logger } from '@outputai/core'; | |||
There was a problem hiding this comment.
💅 @outputai/core gets imported twice now (line 1 and here). Merge them:
import { ValidationError, z, Logger } from '@outputai/core';|
|
||
| if ( !models ) { | ||
| console.warn( 'Failed to fetch models pricing' ); | ||
| Logger.warn( 'Failed to fetch models pricing' ); |
There was a problem hiding this comment.
❓ Logger has no namespace, so these land under the generic Activity logs with nothing saying they came from llm cost. validations.js keeps its [output-llm] prefix but these don't, so it's a bit inconsistent. Worth a short prefix here too so they stay traceable? Small.
There was a problem hiding this comment.
I agree that this isnt ideal. I am adding namespace overwriting on the Logger.
311062a to
9162c95
Compare
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
| The namespace can be customized by setting it in the metadata: | ||
|
|
||
| ```js | ||
| Logger.warn('My awesome message', { namespace: 'Custom' }); |
There was a problem hiding this comment.
❓ wdyt about being able to apply name spaces across all instances? e.g.
HttpLogger = new Logger("http")
HttpLogger.warn("foo")or
HttpLogger = Logger.createLogger("http")
HttpLogger.warn("foo")Seems both more familiar, prevent us from having to remember in each package at each log line what name space to add, and would prevent having the collission issue with the key "namespace:"
There was a problem hiding this comment.
I would steer away from both. I think the is very convenient to be able to just Logger.*() at any point, without dealing with instances. We could have a .bindNamespace() module that returns a logger "instance" with the namespace already set.
There was a problem hiding this comment.
Done. Make de the default Logger an instance with a .createLogger() that creates another instance with bound namespace.
Summary
Loggerfrom Core module in place ofconsole.*.Test plan