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

update formatHttpRequest and formatHttpResponse to handle *client* req and res #67

Open
trentm opened this issue Mar 18, 2021 · 3 comments
Labels
agent-nodejs Make available for APM Agents project planning.

Comments

@trentm
Copy link
Member

trentm commented Mar 18, 2021

Currently formatHttpRequest and formatHttpResponse handle server-side request and response objects (from node core and some of the http frameworks). It would be nice to support the client-side request and response objects:

  • client-side request is a
    http.ClientRequest returned from http.request() and http.get()
  • client-side response is a
    http.IncomingMessage returned from the "response" event of a client http.request()

Normally I'm opposed to DWIM'ing, but it would be nice if it could be managed here without gross heuristics and ambiguity. Node core docs (and likely lots of community docs) commonly refer to both server-side and client-side requests and responses as req and res. Just logging those variables as is would be nice.

One potential issue is if this blows up to a desire to support many varying "client request" and "client response" objects from Node-land http client libraries. I don't know if that would be the case.

FWIW, some minor prior art is handling for client_req and client_res log record fields in Bunyan and client_req and client_res serializers for Bunyan in the restify-clients package.

@github-actions github-actions bot added the agent-nodejs Make available for APM Agents project planning. label Mar 18, 2021
@trentm
Copy link
Member Author

trentm commented Mar 18, 2021

I need to ask/check if client req/res events in ECS "http." fields would cause problems.

@trentm
Copy link
Member Author

trentm commented Mar 22, 2021

FWIW, here is what pino's stdSerializers will serialize for client req/res:

[1616438720932] INFO (14594 on pink.local): finished client req/res
    req: {
      "method": "GET",
      "url": "/",
      "remoteAddress": "127.0.0.1",
      "remotePort": 3000
    }
    res: {
      "statusCode": 200
    }

@trentm
Copy link
Member Author

trentm commented Apr 15, 2021

See this PR for discussion and code for extracting the url field, at least, from a ClientRequest object: elastic/apm-agent-nodejs#2039

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
agent-nodejs Make available for APM Agents project planning.
Projects
None yet
Development

No branches or pull requests

1 participant