-
Notifications
You must be signed in to change notification settings - Fork 2
Use node/express for the c4ai service #22
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
Conversation
PR Change SummaryTransitioned the c4ai service from a Deno app to an Express app, enhancing context propagation and attribute value handling. Implemented biome for linting and formatting, simplifying the development process.
Modified Files
How can I customize these reviews?Check out the Hyperlint AI Reviewer docs for more information on how to customize the review. If you just want to ignore it on this PR, you can add the Note specifically for link checks, we only check the first 30 links in a file and we cache the results for several hours (for instance, if you just added a page, you might experience this). Our recommendation is to add |
This also bumps the logfire package version, fixing the object attributes issue.
1667608
to
7093a01
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a few minor things, overall looks good but I haven't tried it.
c4ai/.zed/settings.json
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can just remove this file now we're not using deno.
c4ai/biome.json
Outdated
}, | ||
"javascript": { | ||
"formatter": { | ||
"quoteStyle": "double" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we use single quotes? it should make the diff smaller in code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and no trailing commas 🙏.
c4ai/main.ts
Outdated
column: number; | ||
} | ||
|
||
function logfireAddSchema( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we move this down to keep the diff small-ish?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just realized what this function does - we should not need it anymore, now that the attributes work.
import * as logfire from "logfire"; | ||
|
||
logfire.configure({ | ||
diagLogLevel: logfire.DiagLogLevel.DEBUG, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need to set service_name
and environment
here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added the following env variables to docker-compose
- isn't this better?
LOGFIRE_ENVIRONMENT: ${LOGFIRE_ENVIRONMENT}
LOGFIRE_SERVICE_NAME: c4ai
The context propagation works:
The complex attribute values are correctly sent:

I'm not certain on the docker setup changes.
Used biome as a all-in-one solution for linting/formatting. I think it is great (and simpler than prettier/eslint) for smaller projects. It should work with Zed, if you have the extension installed.