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

attach request info to log events #28

Merged
merged 20 commits into from Aug 10, 2022

Conversation

schehata
Copy link
Collaborator

when we print logs to vercel console we receive request information
with the logs. Those are missing when we send logs using axiom logger.
This collects the request information and attach them as part of the
fields object, they would be parsed in the backend.

@vercel
Copy link

vercel bot commented Jul 22, 2022

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

Name Status Preview Updated
next-axiom-example ✅ Ready (Inspect) Visit Preview Aug 10, 2022 at 9:36AM (UTC)

src/withAxiom.ts Outdated
ev.waitUntil(log.flush());
return res;
} catch (error) {
log.error('Error in edge function', { error });
report.request.statusCode = 500;
Copy link
Member

Choose a reason for hiding this comment

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

Will this do anything?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

probably would have been useful in case of EDGE_REPORT. I will remove it.

Copy link
Member

Choose a reason for hiding this comment

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

We need to collect logs until flush in functions. Then we can attach the report (including the correct status code) to each log and send them all out.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

hmm this would have very different behavior than frontend I guess. how should we approach this? different logger class that doesn't throttle and send?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I am also wondering if seeing the status of the request on each log is that useful. for checking the status of request, wouldn't one record of that be enough?

export type AxiomMiddleware = (
request: AxiomRequest,
event: NextFetchEvent
) => NextMiddlewareResult | Promise<NextMiddlewareResult>;
Copy link
Member

Choose a reason for hiding this comment

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

I think we'll also need an AxiomApiRequest, the API function logs have the same issue don't they?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

on it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

the NextAPIRequest structure is different than NextRequest. I can't find request ip and geo info for example.

Copy link
Contributor

Choose a reason for hiding this comment

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

Are you still blocked on this @schehata ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I was able to get the IP from the headers, and for the region information I used VERCEL_REGION. I believe we can move forward with those

src/logger.ts Outdated Show resolved Hide resolved
src/withAxiom.ts Outdated Show resolved Hide resolved
src/withAxiom.ts Outdated Show resolved Hide resolved
@schehata
Copy link
Collaborator Author

schehata commented Aug 8, 2022

I was able to attach environment, region, and route to vercel object. e.g:

{
  "level": "info",
  "message": "Hello from function",
  "request": {
    "host": "axiom-local-next.vercel.app",
    "ip": "196.157.32.191",
    "method": "GET",
    "path": "/api/hello",
    "region": "iad1",
    "scheme": "https",
    "statusCode": 200,
    "userAgent": "Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/104.0.0.0 Safari/537.36"
  },
  "vercel": {
    "environment": "production",
    "projectId": "prj_NHRfKvjJ98HnVZ1JQHocgGhqTLOZ",
    "region": "iad1",
    "route": "/api/hello",
    "source": "log",
    "userName": "islam-axiomco"
  }
}

Copy link
Contributor

@lukasmalkmus lukasmalkmus left a comment

Choose a reason for hiding this comment

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

This looks good from my side but I feel like I'm the wrong person to push the button on this. Is there somebody that has been more involved than me?

@schehata
Copy link
Collaborator Author

schehata commented Aug 9, 2022

This looks good from my side but I feel like I'm the wrong person to push the button on this. Is there somebody that has been more involved than me?

Probably only arne. Neil was involved in the payload structure.

@schehata schehata force-pushed the islam/dx-137-attach-request-information-to-log-events branch from 72459ec to 9d03d9a Compare August 10, 2022 09:36
@schehata schehata merged commit ab010df into main Aug 10, 2022
@schehata schehata deleted the islam/dx-137-attach-request-information-to-log-events branch August 10, 2022 15:15
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.

None yet

4 participants