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

Request logging customization #4463

Open
2 tasks done
kibertoad opened this issue Dec 8, 2022 · 17 comments · May be fixed by #4955
Open
2 tasks done

Request logging customization #4463

kibertoad opened this issue Dec 8, 2022 · 17 comments · May be fixed by #4955
Labels
feature request New feature to be added semver-minor Issue or PR that should land as semver minor

Comments

@kibertoad
Copy link
Member

Prerequisites

  • I have written a descriptive issue title
  • I have searched existing issues to ensure the feature has not already been requested

🚀 Feature Proposal

Expose fastify param createRequestLogMessage, which would allow customizing request param log message.

Motivation

Currently logic for request logging looks like this:

    if (disableRequestLogging === false) {
      childLogger.info({ req: request }, 'incoming request')
    }

It is not possible to customize it, which may sometimes be desireable.

Example

It would be great to be able to replace incoming request with a custom-built message. So something like

    if (disableRequestLogging === false) {
      childLogger.info({ req: request }, createRequestLogMessage(req))
    }

createRequestLogMessage would default to a function which returns 'incoming request'.

Full usage example:

  const app = fastify({
    createRequestLogMessage: (req) => { return `${req.id}: incoming request`
  })
@kibertoad kibertoad added enhancement feature request New feature to be added labels Dec 8, 2022
@kibertoad
Copy link
Member Author

If accepted, we could send a PR.

@jsumners
Copy link
Member

jsumners commented Dec 8, 2022

I think it would be better to make it a factory function option that gets cached as we do with other things of this nature. It would remove the function invocation cost at every request and make it a symbol lookup.

Otherwise, seems reasonable to me.

@kibertoad
Copy link
Member Author

a factory function option that gets cached as we do with other things of this nature

@jsumners Can you point to any part of existing code that does this?

@kibertoad
Copy link
Member Author

It would remove the function invocation cost at every request and make it a symbol lookup.

How would that work if we may need to dynamically recalculate the message for every request?

@jsumners
Copy link
Member

jsumners commented Dec 8, 2022

It would remove the function invocation cost at every request and make it a symbol lookup.

How would that work if we may need to dynamically recalculate the message for every request?

Your original statement was suggesting only the replacement of the text string "incoming request". The dynamic nature of the replacement was not explicitly stated. In that case, yes, it would need to be a function.

@kibertoad
Copy link
Member Author

I mentioned that it would make sense to incorporate request details, otherwise not much benefit over original

@Eomm
Copy link
Member

Eomm commented Dec 8, 2022

I would not change that log, since it is much easier adding a onRequest/onSend hook to customize the log message.

The big deal here would be: could we create a plugin that adds those hooks AND takes control of the disableRequestLogging server's option?
Right now, it is not possible, but often I would like to customize the server's option from a plugin

@Eomm Eomm added semver-minor Issue or PR that should land as semver minor and removed enhancement labels Apr 1, 2023
@kdrewes
Copy link

kdrewes commented Jul 28, 2023

Hello, I want to attempt to tackle this issue. Please let me know if this issue no longer needs assistance.

@mcollina
Copy link
Member

Sure, how do you plan to tackle this issue?

@kdrewes
Copy link

kdrewes commented Jul 31, 2023

Sure, how do you plan to tackle this issue?

Ok thanks! I would like to create a request param log message which will provide a detailed overview of each request, ideally something that is similar to the layout of Window's Event Viewer. I also think kibertoad's original idea to create a new function called createRequestLogMessage(req) is perfect and I would like to incorporate that in the project if possible. Therefore, here's how I would like the message to be structured:

------------------------------------ Incoming Request ------------------------------------

| Date and Time | HTTP Method | Request IP Address | Request URL |

new Date or 		req.method			req.ip					req.url
req.timestamp 

————————————————————————————————————————————————-

Or maybe have the layout look like something similar to this:



------------------------------------ Incoming Request ------------------------------------
Date and Time: new Date() or req.timestamp

HTTP Method: req.method

Request IP address: req.ip



Request URL: req.url

————————————————————————————————————————————————-

The syntax would be located in fastify.js, unless advised elsewhere, and would look similar to this:


const date = new Date();




// Please let me know if you’d prefer req.timestamp instead
const DateAndTime = {
seconds: date.getSeconds(),
minutes: (date.getMinutes() < 10) ? '0' + date.getMinutes() : '' + date.getMinutes(),
hours: (date.getHours() >= 13) ? date.getHours() % 12 : date.getHours(),
days : date.getDate(),
months: date.getMonth() + 1,
years: date.getFullYear(),
meridiem: date.getHours() >= 12 ? 'PM' : 'AM',
};


const LogData = {


incomingRequest : '------------------------------------ Incoming Request ------------------------------------'
categories : '| Date and Time | HTTP Method | Request IP Address | Request URL |'
underLine : ‘————————————————————————————————————————————————-‘
}

const app = fastify({
createRequestLogMessage: (req,DateAndTime,LogData) => {



// Declare timestamp variable
const timestamp = ${DateAndTime.months}/${DateAndTime.days}/${DateAndTime.years} ${DateAndTime.hours}:${DateAndTime.minutes}:${DateAndTime.seconds} ${DateAndTime.meridiem}

const properties = `	${timestamp}		${req.method}	${req.ip}		${req.url}	`;

return '${LogData.incomingRequest}\n${LogData.categories}\n${properties}\n${LogData.underLine}'

}
});

Please let me know if you have any recommendations or critiques and I will make sure to adjust my plan accordingly. Thank you again.

@metcoder95
Copy link
Member

The createRequestLogMessage signature looks sound. Have you considered only passing the request object? At that point in time, the Request instance has been made, for instance, most of the information from the request is already parsed and we pass it to the function to enable it to create its own message.

What do you think?

@kdrewes
Copy link

kdrewes commented Jul 31, 2023

The createRequestLogMessage signature looks sound. Have you considered only passing the request object? At that point in time, the Request instance has been made, for instance, most of the information from the request is already parsed and we pass it to the function to enable it to create its own message.

What do you think?

Hi @metcoder95, I agree that's a good point thank you for letting me know.

Please let me know what you think of my revised version:




const app = fastify({
createRequestLogMessage: (req) => {



const date = new Date();




// Please let me know if you’d prefer req.timestamp instead
const DateAndTime = {
seconds: date.getSeconds(),
minutes: (date.getMinutes() < 10) ? '0' + date.getMinutes() : '' + date.getMinutes(),
hours: (date.getHours() >= 13) ? date.getHours() % 12 : date.getHours(),
days : date.getDate(),
months: date.getMonth() + 1,
years: date.getFullYear(),
meridiem: date.getHours() >= 12 ? 'PM' : 'AM',
};


const LogData = {


incomingRequest : '------------------------------------ Incoming Request ------------------------------------'
categories : '| Date and Time | HTTP Method | Request IP Address | Request URL |'
underLine : ‘————————————————————————————————————————————————-‘
}


// Declare timestamp variable
const timestamp = ${DateAndTime.months}/${DateAndTime.days}/${DateAndTime.years} ${DateAndTime.hours}:${DateAndTime.minutes}:${DateAndTime.seconds} ${DateAndTime.meridiem}

const properties = ${timestamp} ${req.method} ${req.ip} ${req.url} ;

return ${LogData.incomingRequest}\n${LogData.categories}\n${properties}\n${LogData.underLine};
}
});

Or perhaps

const app = fastify({
createRequestLogMessage: (req) => {




const LogData = {


incomingRequest : '------------------------------------ Incoming Request ------------------------------------'
categories : '| Date and Time | HTTP Method | Request IP Address | Request URL |'
underLine : ‘————————————————————————————————————————————————-‘
}


const properties = ${req.timestamp} ${req.method} ${req.ip} ${req.url} ;

return ${LogData.incomingRequest}\n${LogData.categories}\n${properties}\n${LogData.underLine};
}
});

Please let me know if you have any questions. Thank you.

@mcollina
Copy link
Member

the code above is not very readable.

Go ahead and send a PR but do not alter the current message.

@kdrewes
Copy link

kdrewes commented Jul 31, 2023

the code above is not very readable.

Go ahead and send a PR but do not alter the current message.

My apologies I'll make sure to have it more organized moving forward. But Just to make sure would the current message I don't want to alter be:

if (disableRequestLogging === false) { childLogger.info({ req: request }, 'incoming request') }

Thanks.

@metcoder95
Copy link
Member

metcoder95 commented Jul 31, 2023

As @mcollina pointed out, might be better to see a PR

But Just to make sure would the current message I don't want to alter be:

if (disableRequestLogging === false) { childLogger.info({ req: request }, 'incoming request') }

Yes, if the createRequestLogMessage is not passed, the current message should prevail

@kdrewes
Copy link

kdrewes commented Jul 31, 2023

Ok gotcha, I'll make sure to leave that the way it is. Please let me know if you have anymore questions about this and thank you again for all your help.

@kdrewes kdrewes linked a pull request Aug 4, 2023 that will close this issue
4 tasks
@Eomm Eomm linked a pull request Aug 5, 2023 that will close this issue
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request New feature to be added semver-minor Issue or PR that should land as semver minor
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants