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: add telemetry to the chat API route #26

Merged
merged 18 commits into from
Aug 29, 2024
Merged

Conversation

stefl
Copy link
Contributor

@stefl stefl commented Aug 29, 2024

Description

  • This adds telemetry just to the /api/chat route
  • Breaks up the logic into separate functions to reduce cognitive load (thank you Sonar)
  • Includes the ability to mock the LLM calls for categorisation and chat in the Aila initialisation step
  • Verifies that we receive telemetry events for the Aila Generate step, but nothing else within the Aila package yet

Unfortunately it seems that Datadog relies on deploying the Datadog Agent in GCP. So until we have that, I've set up some log tracing and a log drain in Vercel to send logs to Datadog.

This means we get to see timings in Datadog, but we will need someone to set up the agent for us as a separate task. Once that's done, we will need to set up some environment variables in Doppler for the Datadog Agent URL etc.

When that's done, we can remove the logging and rely on the agent instead to send logs to the Tracing part of the Datadog UI instead of to the Logs part.

Copy link

vercel bot commented Aug 29, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
oak-ai-lesson-assistant ✅ Ready (Inspect) Visit Preview 💬 Add feedback Aug 29, 2024 6:17pm

@@ -0,0 +1,255 @@
import {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

By breaking this out into a separate file we can write tests without needing to deal with Clerk / Middleware etc.

throw e;
}

export async function handleChatPostRequest(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The actual chat post request function now is much simpler and errors are handled in the above methods, which also report telemetry

Copy link
Collaborator

Choose a reason for hiding this comment

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

This function is now so easy to follow at a high level. A lot easier to understand the steps involved


import { streamingJSON } from "./protocol";

export async function handleUserLookup(span: Span, id: string) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The user lookup is now in a separate method that we can mock

@@ -0,0 +1,14 @@
export async function consumeStream(stream: ReadableStream): Promise<string> {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Helper for consuming the chat stream

package.json Outdated Show resolved Hide resolved
Copy link

github-actions bot commented Aug 29, 2024

Playwright e2e tests

Job summary

Download report

To view traces locally, unzip the report and run:

npx playwright show-report ~/Downloads/playwright-report

@stefl stefl changed the title Feat/telemetry chat api feat: add telemetry to the chat API route Aug 29, 2024
Copy link
Collaborator

@codeincontext codeincontext left a comment

Choose a reason for hiding this comment

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

Looks good to me! I'd be keen to get this merged as soon as we can so we can be sure that there aren't any regressions and we don't have to make any changes to how we're recording

throw e;
}

export async function handleChatPostRequest(
Copy link
Collaborator

Choose a reason for hiding this comment

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

This function is now so easy to follow at a high level. A lot easier to understand the steps involved

Comment on lines 20 to 28
if (!result) {
userLookupSpan.setTag("error", true);
userLookupSpan.setTag("error.message", "user lookup failed");
return {
failureResponse: new Response("User lookup failed", {
status: 400,
}),
};
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I can't see how this state is possible. Can we just remove this clause?

apps/nextjs/src/app/api/chat/user.ts Show resolved Hide resolved
Copy link

@stefl stefl merged commit 936641a into main Aug 29, 2024
12 checks passed
@stefl stefl deleted the feat/telemetry_chat_api branch August 29, 2024 18:22
@oak-machine-user
Copy link
Collaborator

🎉 This PR is included in version 1.3.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants