[Discuss] Duplicate logs when unifying behaviour in logging HTTP 500 errors

[afharo]

This is a follow up to #65291.

Whenever a route handler throws, the HTTP service logs the error and responds with 500 - Internal Error. However, when a handler returns res.internalError() without providing any arguments, it can only log “Error: Internal Server Error”, which is far from helpful.

That’s why we’re planning to deprecate that method in favour of throwing (#85541). We are hoping to normalize the user experience where Kibana logs all the 500 errors in the same fashion.

This raises the following concern: when a handler logs the error itself and throws (so the HTTP service logs the error again) Kibana logs 2 entries with the same message but different logger context [plugins, pluginName] and [server, http] respectively.

We've considered adding the pluginName to the [server, http], and removing the plugin's log entry. But that might be a breaking change to users that might have specific appenders to any of the existing logger contexts.

On top of that, there are cases where the plugin's logger has an extended context that might be useful to our users:

kibana/x-pack/plugins/security/server/authentication/index.ts

Lines 110 to 113 in 2782204

	} catch (err) {
	authLogger.error(err);
	return response.internalError();
	}

mind the use of authLogger, which is different from the "base" plugin logger

What do you think? Are we OK with double-logging in favour of more user flexibility?

[elasticmachine]

Pinging @elastic/kibana-core (Team:Core)

[pgayvallet]

We've considered adding the pluginName to the [server, http], and removing the plugin's log entry.

I would go with that, and just document that behavior

But that might be a breaking change to users that might have specific appenders to any of the existing logger contexts.

I doubt we have a lot of users (if any) currently using specific appenders, as the whole API is currently mostly undocumented for our end users, so I would say this seems acceptable?

[afharo]

I doubt we have a lot of users (if any) currently using specific appenders, as the whole API is currently mostly undocumented for our end users, so I would say this seems acceptable?

IMO, I would consider Observability ES filters also as appenders: I'm thinking on a workflow where a customer ships all the logs from Kibana to an ES index. Then they have filters for specific contexts (not sure if customers actually follow this workflow, but in my mind, it feels like an easy process). Maybe, I'm just being paranoid here. If so, happy with the breaking-not-so-breaking change 😉

[mshustov]

We've considered adding the pluginName to the [server, http], and removing the plugin's log entry.

I'm not sure it's easy to add pluginName to server.http context. server.http works without complaints so far.

But that might be a breaking change to users that might have specific appenders to any of the existing logger contexts.

We haven't announced the new logging system as stable so far. Also, we changed the plugin logging context several times during migration, so I also think it's acceptable.

[afharo]

I've updated the original PR to remove the places where the plugins are logging the errors themselves. If there are any concerns, they'll likely discuss it in there. I'll link this issue to that PR so, when merged, this issue will be closed as well.