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: middleware #228

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

feat: middleware #228

wants to merge 5 commits into from

Conversation

ImLunaHey
Copy link
Contributor

@ImLunaHey ImLunaHey commented Aug 20, 2024

this adds middleware that will allow users to proxy their logs and webvital reports to their backend avoiding the need to provide their Axiom token and dataset to the frontend.

Copy link

vercel bot commented Aug 20, 2024

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

Name Status Preview Comments Updated (UTC)
next-axiom-example ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 8, 2025 0:59am

@ImLunaHey ImLunaHey requested a review from dasfmi August 20, 2024 00:09
@ImLunaHey ImLunaHey marked this pull request as ready for review August 20, 2024 00:09
@grrowl
Copy link

grrowl commented Sep 25, 2024

random request: could the /_axiom path be passed in as an option? avoid it being blocked in public tracking rulesets.

Copy link
Collaborator

@dasfmi dasfmi left a comment

Choose a reason for hiding this comment

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

Overall looks good, but I have couple of points.

  • Its missing documentation on how to use it.
  • Perhaps now we can get deprecated usage of NEXT_PUBLIC_AXIOM_TOKEN and update the docs to mention AXIOM_TOKEN instead.
  • There is now a custom endpoint added by @c-ehrlich, not sure if we need to handle anything related to that or the config takes care of it.

const webVitalsEndpoint = axiomConfig.getIngestURL(EndpointType.webVitals);
const logsEndpoint = axiomConfig.getIngestURL(EndpointType.logs);

const headers = {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Our fetch sends a special UserAgent header to identify which SKS is sending the request. Its missing in here. Perhaps you can just proxy the request headers and adds the token on top of?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Addressed this in the last commit

);

// Return a 204 to the client
return new NextResponse(null, { status: 204 });
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't status be returned as error if the request fails? Users might not notice if it always returns 204 even if it fails.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The last commit checks if .waitUntil is defined and, if not, awaits the response. We can decide not to use .waitUntil altogether for this case since we are pretty much using the middleware like an API route here and not redirecting or continuing execution after it finishes the fetch

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.

4 participants