-
Notifications
You must be signed in to change notification settings - Fork 16
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 axiom logger, nextjs and react libraries #257
base: main
Are you sure you want to change the base?
Conversation
c7980c7
to
4c3bcc7
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.
I would rename the core
package to match the name in package.json
, so that it won't lead to confusion.
Changed this, now the code is under |
2d7ab39
to
6c45268
Compare
@axiomhq/js
@axiomhq/logging
@axiomhq/pino
@axiomhq/react
@axiomhq/nextjs
@axiomhq/winston
commit: |
e1a5bac
to
8f8203a
Compare
@@ -0,0 +1,12 @@ | |||
import { Logger } from '@axiomhq/logging'; | |||
import { AxiomProxyTransport, ConsoleTransport } from '@axiomhq/logging/transports'; |
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.
would it be easier if we simplify this import to:
import { Logger, AxiomProxyTransport, ConsoleTransport } from '@axiomhq/logging';
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.
Don't think so, having the transports in a separate/grouped export feels like a good separation. Reminds me of how vercel does the AI sdk, you import core functionality from the "ai" package, but need to import your models from a separate package.
ex:
import { generateText } from "ai"
import { mistral } from "@ai-sdk/mistral"
const { text } = await generateText({
model: mistral("mistral-large-latest"),
prompt: "What is love?"
})
it has the benefit of allowing users to have a slimmer list of options in their autocomplete when importing from the main package, mainly functionalities, and have a clear place to get these things that serve a very specific purpose that you need to pass in conjunction with other functions (transports/models)
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.
@bdsqqq I agree to the concept, and that's what we are trying to do with the package, but the transports plays a core value (main funcationality) for the logging package and there is like 2 or 3 of them maximum. The Ai ones makes sense because they are different providers that would be implemented/developed separately as apposite to the transports here. wdyt?
"author": "", | ||
"license": "ISC", | ||
"dependencies": { | ||
"@axiomhq/js": "^1.3.1" |
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 the version here needs to be "workspace:*"
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.
@gabrielelpidio this is still mentioning a fixed version instead of workspace version.
@@ -0,0 +1,44 @@ | |||
{ |
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.
Make sure to go over all the package.json files in the PR and bring them closer to whats in axiom-js package.json in terms of license, contributors, homepage, repo, etc.
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.
It should be good now; check it out @dasfmi
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.
@gabrielelpidio great work, I left some comments but they are small ones, in general looks one step closer to 100%.
One more question, how do we handle the notFound and redirect errors for Next.js? We will need to address the request for setting defaults somehow. |
This was top of mind when doing the API for route handlers. Everything is customizable, from error handling to the ingested flow; in this case, we provide a default
Internally, the function does the following: axiom-js/packages/nextjs/src/routeHandler.ts Lines 84 to 96 in 86b3f3e
This means developers can create their own function, with different LogLevels depending on their needs. Deeper look (relevant to the question, but not necessary to change the LogLevel):On the other side, capturing the status code is done internally by the axiom-js/packages/nextjs/src/routeHandler.ts Lines 72 to 82 in 86b3f3e
All this also means that the function that handles errors and the default callback are just coupling these pieces together, and if the developer wants they can use as many helpers as they want from what we export axiom-js/packages/nextjs/src/routeHandler.ts Lines 33 to 48 in 86b3f3e
axiom-js/packages/nextjs/src/routeHandler.ts Lines 98 to 110 in 86b3f3e
All of this lives in a single file under /packages/nextjs/src/routeHandler.ts |
df13365
to
e682005
Compare
export type LoggerConfig = { | ||
args?: { [key: string]: any }; | ||
transports: [Transport, ...Transport[]]; | ||
logLevel?: LogLevel; |
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.
Make sure log level works if it's gonna be included
eda0753
to
4095fc1
Compare
4095fc1
to
eb565a0
Compare
"author": "", | ||
"license": "ISC", | ||
"dependencies": { | ||
"@axiomhq/js": "^1.3.1" |
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.
@gabrielelpidio this is still mentioning a fixed version instead of workspace version.
@@ -0,0 +1,14 @@ | |||
import { Transport } from '.'; | |||
import { SimpleFetchTransport } from './fetch'; | |||
interface AxiomProxyConfig { |
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.
nitpick: insert a new line before interface.
flush: () => Promise<void> | void; | ||
} | ||
|
||
export * from './axiom-js'; |
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 don't like how the index file is import/export other transports, and the other transports are importing the Transport interface, cycle dependencies are always weird. I would prefer if the Transport interface lived in its own file in the src folder.
"vitest": "^0.34.6" | ||
}, | ||
"peerDependencies": { | ||
"next": "15.1.4" |
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.
Perhaps you can be more flexible with the versioning here, something like >=15.1
"logging" | ||
], | ||
"dependencies": { | ||
"@axiomhq/logging": "workspace:*" |
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.
Wondering if this should this be a peer dependency or not? No strong opinion though
No description provided.