Add Telemetry abstraction and remove Sentry as a peer dep #46
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
Note: the pnpm lockfile was also updated to the latest v9. |
| >; | ||
|
|
||
| export class ChatModel< | ||
| CustomCtx extends Model.Ctx = Model.Ctx, | ||
| MTelemetry extends Telemetry.Base = Telemetry.Base, |
There was a problem hiding this comment.
@rileytomasek I tried both ways with the Ctx generic coming before telemetry and after, and I think after is the right call. Most of the time you want telemetry to be inferred by the compiler, whereas it's more common to customize the context type.
| ); | ||
| return response; | ||
| } | ||
| return this.telemetry.startSpan( |
There was a problem hiding this comment.
the only reason this block looks like a bigger change than it is is due to spacing. it's just replacing Sentry with this.telemetry.
|
|
||
| await Promise.allSettled( | ||
| this.events.onComplete?.map((event) => | ||
| this.events.onStart?.map((event) => |
There was a problem hiding this comment.
github's diff is off here; these lines don't actually match
|
@rileytomasek maybe we don't even need all of the |
There was a problem hiding this comment.
This is amazing — thanks @transitive-bullshit! I'll test it out locally in Dexa's codebase to see how it goes.
I'd rather drop all of the generics if possible, but will see if we actually need them and report back.
This is a simplified version of #46. It was easier to copy the important parts and start clean than try and delete all of the generics code. Replace the direct dependency on Sentry with the abstract `Telemetry` interface from @transitive-bullshit. This should have no impact on users who weren't using the Sentry integration, and requires passing Sentry as an argument when creating a model instance. ```ts import { ChatModel } from '@dexaai/dexter' import * as Sentry from '@sentry/node' const model = new ChatModel({ telemetry: Sentry }) ```
|
@transitive-bullshit I tested locally against Dexa source and there were no issues 👌🏻. I can't think of a scenario where we need the generics for |
* Add telemetry abstraction to remove Sentry dep This is a simplified version of #46. It was easier to copy the important parts and start clean than try and delete all of the generics code. Replace the direct dependency on Sentry with the abstract `Telemetry` interface from @transitive-bullshit. This should have no impact on users who weren't using the Sentry integration, and requires passing Sentry as an argument when creating a model instance. ```ts import { ChatModel } from '@dexaai/dexter' import * as Sentry from '@sentry/node' const model = new ChatModel({ telemetry: Sentry }) ``` * Rename `Telemetry.Base` to `Telemetry.Provider`
Desc
Extends #38 to no longer depend on
@sentry/node, but rather a flexible, base, sentry-like telemetry provider.By default, models will use a
DefaultTelemetryif not given a telemetry provider like@sentry/node.I added a unit test to ensure that at least the types of
@sentry/nodematch the expectedTelemetry.Baseprovider type.@rileytomasek I'm sure there may be a cleaner way to do this, but I felt like the telemetry provider belonged outside of
modelsince it could theoretically be used fordatastorein the future. At the very least, this change lets us continue iterating on Dexter without it being dependent on Sentry, which I think is a net win.Example