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

Add more information to default logs #121

Closed
davidebianchi opened this issue Jun 28, 2021 · 6 comments
Closed

Add more information to default logs #121

davidebianchi opened this issue Jun 28, 2021 · 6 comments
Labels
enhancement New feature or request

Comments

@davidebianchi
Copy link
Member

davidebianchi commented Jun 28, 2021

The feature you are proposing

For the default log created by the library (incoming request and request completed), I'd like to add some custom values which help to identify the scope of the api request.
Could be interesting also adds the request path params in a specific object

The description of the bug or the rationale of your proposal

Logs are used to aggregate and analyze information through some specific tool to visualize the information. In this way, it should be possible to aggregate and use information in a more easy way without add additional (and possibly not very useful) logs.

A snippet of code for replicating the issue or showing the proposal usage if applicable

If it is exposed an api /foo/:id, and a request is performed as /foo/my-id, the object of the log could be something like:

{
  ...
  "url": {
      ...
      "params": {
        "id": "my-id"
      }
    }
}

In addition, it could be possible to add an additional fields through the request object.

In the handler, could be available a function to add some custom params to log:

req.setAdditionalInfoToLogRequestCompleted((defaultLog, reply) => {
  return {
	...defaultLog,
    moreInfo: true
  }
})

or something similar

@fredmaggiowski
Copy link
Member

I like the idea, as far as I understood you are actually proposing two improvements on the same feature:

  1. logging request.params

I think fastify already provides us a nice way to gather all the request params so I think that this could be really easy to implement, i worry they might have sensitive data, however we already log the full path hence there'd be no issue in adding params (we might have to redact specific params in the future)

  1. additional log parameters

As far as I understood you wish that the user could provide custom properties to be added to the library logs, however the proposed snippet interface would be to let the user define a function that receives the full log generated and the reply giving the user a way to completely alter the library log and/or use the reply to actually send content to the client. I think this interface could be abused, I can think of two possible alternatives:

  • the user defines a function whose paramters are req and (a copy/subset of) reply: the function has to return an object that will be (shallow/deeply?) merged with the log
  • the user configures the logger at startup with a set of key-values that are going to be injected in each and every log

What do you think about these possibilities? I think they might respond to different use-cases and are not mutually exclusive, we could implement them both.

@davidebianchi
Copy link
Member Author

davidebianchi commented Jun 28, 2021

  1. yes, I'd like to add the params object created by fastify. The value of the object should be a string. I think about the sensitiveness, but we already log it so it is not a problem as of now.

  2. Why do not leave the user to completely rewrite the log? I suppose this to be an advanced functionality, so I'd like the user could change the log as he want (at his own risk).

For the two possible alternatives:

  • in the request decorator, req should already be available in the context (so it should be accessible using this). But we could maybe make it explicit. I'd prefer to not handle the merge, so we could set the http key as not overwritable. Why keep it so strict?
  • the second alternative is not clear. How the user set key-values at startup?

@fredmaggiowski
Copy link
Member

fredmaggiowski commented Jun 28, 2021

I'd prefer to not handle the merge, so we could set the http key as not overwritable. Why keep it so strict?

I agree on preferring not to dealing with the merge, however I don't like it too much to allow the user to alter the log entirely, the purpose of this library is to provide a standard, if the user can do whatever it wants then the library (partly) loses its purpose.
We could pick all returned properties and then filter the standard ones we consider readonly (such as http as you suggested -I don't know if there's some other properties that could receive the same treatment-)

As per the second alternative I didn't express myself properly: the user could provide on init the path to keys in the request/etc that it wishes to be logged, it's our job then to find them and include them in the log (in a position specified by the user).

The user could also provide some static key/value pairs that could be injected in all the logs but I don't see if we have a use case for this kind of feature right now.

@davidebianchi
Copy link
Member Author

For the key/value pairs to add in every log, it was my initial idea: decorate the request with an additionalDefaultLogParam field, and reuse in initial and final request logs.
But a function is way more customizable, and user could control fields in logs more in detail.

For the rewrite, I think about this use case for example.
With this feature, I could decide to write in my incoming request (which is in trace) the request body and headers. And I suppose it should be written in http.request.requestBody, but it is not possible if http is read only.

@davidebianchi
Copy link
Member Author

davidebianchi commented Jul 13, 2021

I create a PR to have something to think about, only for the request completed log (#125).

@davidebianchi
Copy link
Member Author

I close the issue, since the implementation for the request completed log is released. Reopen it if necessary also in incoming request.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants