Additional configuration options for RequestLogger and Logger middleware

[aldas]

• Add middleware.RequestLoggerConfig.HandleError configuration option to handle error within middleware with global error handler thus setting response status code decided by error handler and not derived from error itself.
• Add middleware.LoggerConfig.CustomTagFunc so Logger middleware can add custom text/fields etc to logged (JSON or whatever format) row.

	e.Use(middleware.LoggerWithConfig(middleware.LoggerConfig{
		Format: `{"method":"${method}",${custom}}` + "\n",
		CustomTagFunc: func(c echo.Context, buf *bytes.Buffer) (int, error) {
			return buf.WriteString(`"tag":"my-value"`)
		},
	}))

[codecov]

Codecov Report

Base: 92.38% // Head: 92.69% // Increases project coverage by +0.30% 🎉

Coverage data is based on head (bce3b19) compared to base (be23ab6).
Patch coverage: 72.72% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2341      +/-   ##
==========================================
+ Coverage   92.38%   92.69%   +0.30%     
==========================================
  Files          37       37              
  Lines        4454     4460       +6     
==========================================
+ Hits         4115     4134      +19     
+ Misses        247      238       -9     
+ Partials       92       88       -4     

Impacted Files	Coverage Δ
context.go	88.88% <ø> (+0.19%)	⬆️
echo.go	95.24% <ø> (ø)
middleware/logger.go	84.89% <40.00%> (-1.68%)	⬇️
middleware/request_logger.go	97.56% <100.00%> (+0.08%)	⬆️
ip.go	100.00% <0.00%> (ø)
middleware/compress.go	81.15% <0.00%> (ø)
middleware/body_dump.go	90.47% <0.00%> (ø)
echo_fs.go	92.50% <0.00%> (+7.55%)	⬆️
... and 1 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

[aldas]

@vishr or @lammel do you have time? If not, I'll merge this in couple of days :)

[lammel]

@vishr or @lammel do you have time? If not, I'll merge this in couple of days :)

Time is quite limited, sorry for that. We'll take a look soon ;)

[aldas]

@lammel please re-review

[lammel]

Thinking about the implications of the custom tag I find arbitrary binary data being modified for a custom tag having the potential to shout yourself in the foot and write invalid JSON because of typos or dangling commas.

As it's only for logging it will only affect the logserver in the background. The HandleError directive might cause unexpected side-effects which might be hard to debug.

I tend to approve, as defaults are sane and people should know what the are doing.
We might emphasize that more in the code comments with a "WARNING" for both new options.

[aldas]

Thinking about the implications of the custom tag I find arbitrary binary data being modified for a custom tag having the potential to shout yourself in the foot and write invalid JSON because of typos or dangling commas.

There is comment for that field // Make sure that outputted text creates valid JSON string with other logged tags.. Possiblity to shoot you foot is pretty much with every feature. This tag is handy because people occasionally want their own custom logic and there limit at some point how many niche tag should this logger middleware have.

As it's only for logging it will only affect the logserver in the background. The HandleError directive might cause unexpected side-effects which might be hard to debug.

HandleError is necessary evil for some cases. This is because when in request life cycle the RequestLogger receives the returned error and when that error is actually handled. So currently global error handler comes after middlewares and therefore if you have custom built global error handler that decides different status codes than what error might contain - we have a problem. RequestLogger logs code XXX but global error handler decides and send YYY to the client. Only way to have them in sync is to allow developer to opt into calling global error handler inside this middleware. This is similar like Logger middleware does but Logger middleware does not give you a option - it does it always.

[lammel]

There is comment for that field // Make sure that outputted text creates valid JSON string with other logged tags.. Possiblity to shoot you foot is pretty much with every feature. This tag is handy because people occasionally want their own custom logic and there limit at some point how many niche tag should this logger middleware have.

Is using a dedicated interface instead of binary data for the customTag field an option?
It would at least allow to ensure valid JSON (or whatever dataformat is used).

                // CustomTagFields may hold additional custom fields or text that is replaced for the `${custom}` tag to write custom user data into the output buffer
		// Optional.
		CustomTagFields *CustomTagFields

The CustomTagFields would hold a map[string]any and needs an Add() function and serializer methods like MarshalJSON or MarshalText.
Would that serve the purpose too? Might be a little overkill though.

HandleError is necessary evil for some cases. This is because when in request life cycle the RequestLogger receives the returned error and when that error is actually handled. So currently global error handler comes after middlewares and therefore if you have custom built global error handler that decides different status codes than what error might contain - we have a problem. RequestLogger logs code XXX but global error handler decides and send YYY to the client. Only way to have them in sync is to allow developer to opt into calling global error handler inside this middleware. This is similar like Logger middleware does but Logger middleware does not give you a option - it does it always.

Agreed. Now it's even documented here :)

[aldas]

I think interface would be over complicating this thing. Moreover - that log row does not need to be even formatted as JSON. You could create template that has Apache or Nginx request log format.

and having multiple tags as map/slice means that these custom tags could overlap with potential tags that we could add in future. by adding single one - we reserve single tag and give us room to add tag with any name in future. If user want to write multiple tags - he/she can do this within this customTagFunc callback.

[lammel]

Then let's keep it simple for now. Approved!