Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: setup a basic vercel sdk instrumentation #19

Merged
merged 3 commits into from
May 6, 2024

Conversation

Granipouss
Copy link

@Granipouss Granipouss commented May 2, 2024

See tests for usage examples.

For now it just create generations with metrics, inputs and outputs.

TODO (maybe not all now)

  • Handle steps/threads passed as arguments when using generateText or streamText
  • Add tests with another provider (anthropics ?)
  • Add tests with structured generation
  • Add tests with images
  • Add tests with tool calls (it seems there are no functions in vercel sdk)
  • Find a non-chat completion and add tests with it
image

This change is Reviewable

@Granipouss Granipouss force-pushed the brendan/vercel-sdk branch from 934c1f7 to 845c8de Compare May 3, 2024 08:36
@Granipouss Granipouss changed the title feat: setup a barebone vercel sdk instrumentation feat: setup a basic vercel sdk instrumentation May 3, 2024
Copy link
Contributor

@clementsirieix clementsirieix left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The vercel sdk is adding a lot of dependencies, have we checked the size of the build?

Reviewed 4 of 6 files at r1, all commit messages.
Reviewable status: 4 of 6 files reviewed, 2 unresolved discussions (waiting on @Granipouss and @willydouhard)


src/instrumentation/vercel-sdk.ts line 64 at r1 (raw file):

  delete settings.prompt;
  delete settings.abortSignal;
  return settings;

Won't it work to simply use omit(settings, ["model", "prompt", "abortSignal"])?
Also in that case we probably do not need this method for that purpose.


src/instrumentation/vercel-sdk.ts line 123 at r1 (raw file):

      ttFirstToken = Date.now() - startTime;
    }
    outputTokenCount += 1;

Question: I thought a chunk is not strictly equal to a single token?

@Granipouss
Copy link
Author

The dependencies are added as peerDeps and devDeps so it shouldn't impact the final build

@Granipouss
Copy link
Author

src/instrumentation/vercel-sdk.ts line 64 at r1 (raw file):

Previously, clementsirieix (Clément Sirieix) wrote…

Won't it work to simply use omit(settings, ["model", "prompt", "abortSignal"])?
Also in that case we probably do not need this method for that purpose.

Like lodash omit? DO we have an helper for it ?

@Granipouss
Copy link
Author

src/instrumentation/vercel-sdk.ts line 123 at r1 (raw file):

Previously, clementsirieix (Clément Sirieix) wrote…

Question: I thought a chunk is not strictly equal to a single token?

No idea, I just took the logic from the openAI instrument

Copy link
Contributor

@clementsirieix clementsirieix left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 2 of 6 files at r1.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @Granipouss)

@Granipouss Granipouss merged commit 569a63b into main May 6, 2024
3 checks passed
@Granipouss Granipouss deleted the brendan/vercel-sdk branch May 6, 2024 11:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants