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

init sveltekit library #198

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

init sveltekit library #198

wants to merge 8 commits into from

Conversation

dasfmi
Copy link
Collaborator

@dasfmi dasfmi commented Jun 10, 2024

Create a library to support logging from sveltekit.

@dasfmi dasfmi self-assigned this Jun 10, 2024
@jerriclynsjohn
Copy link

I'm looking at the example code, and I want this. Been waiting for this for a while. Let's go @schehata!!!!

@jerriclynsjohn
Copy link

I'd also like to know how this will work with my Sentry integration! How will Sentry and Axiom handle the handleError hook together?



console.log({ token: AXIOM_TOKEN, dataset: AXIOM_DATASET, url: AXIOM_URL })
const logger = new Logger({ token: AXIOM_TOKEN, dataset: AXIOM_DATASET, url: AXIOM_URL }, { runtime: resolveRuntime(), dev, browser, version });

Choose a reason for hiding this comment

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

The logger expects a single object; there are two objects here.

Choose a reason for hiding this comment

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

Also, we could add dev browser, and version as optional properties of LoggerConfig

Choose a reason for hiding this comment

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

It makes sense to pass it as args when calling the logger.error/info than the above.

@jerriclynsjohn
Copy link

I'd also like to know how this will work with my Sentry integration! How will Sentry and Axiom handle the handleError hook together?

Figured it out. I can pass custom error handler to handleErrorWithSentry()


export type LoggerConfig = {
args?: { [key: string]: any };
url?: string;

Choose a reason for hiding this comment

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

This url can also be ditched as this is never used here and is only used at the api/axiom endpoint created by the user.

Choose a reason for hiding this comment

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

We can also add dev, browser, and version here to make the server-side logs parity. In both versions, we can get rid of the browser arg and hardcode it like you've done here for runtime.

Choose a reason for hiding this comment

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

I think it makes sense to pass dev, browser, and version when calling logger.error/info instead of the above method, which essentially does nothing.

@jerriclynsjohn
Copy link

@schehata I have made some changes to the codebase based on the discussion in this PR. I've also made PR #207 for it.

@dasfmi dasfmi marked this pull request as ready for review July 8, 2024 07:45
@dasfmi
Copy link
Collaborator Author

dasfmi commented Jul 8, 2024

@jerriclynsjohn I have update the README, to give us a starting doc. We will have to improve, and hopefully @tothmano when he is back.

There is 2 things that we need to do, and then its ready in my opinion:

  • Think of how users will import the logger in their pages/components and provide & example
  • Implement web-vitals and send them to Axiom. We can probably use web-vitals npm package, the question is, where in a svelte-app would this live?

@dasfmi
Copy link
Collaborator Author

dasfmi commented Jul 8, 2024

aaah, the build process is trying to send logs to Axiom, not sure if we want that:
https://github.com/axiomhq/axiom-js/actions/runs/9838268413/job/27157798727?pr=198

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.

2 participants