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

[http] Add log for route path access #152621

Merged
Merged
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -516,6 +516,7 @@ export class HttpServer {
}

private configureRoute(route: RouterRoute) {
const optionsLogger = this.logger.get('http', 'server', this.name, 'options');
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I chose to add a new logger to make it easier to filter for these specific log lines

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we want to avoid repeating the http and server part, I think we can define the logger asthis.log.get(this.name, 'options);.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We'd lose the full context around where the log comes from if we remove http and server, which makes it very hard to analyze the debug-level logs.
Personally, I'd prefer being able to filter logs by a full logger name that keeps some sort of record of where it's coming from.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We'd lose the full context around where the log comes from if we remove http and server, which makes it very hard to analyze the debug-level logs.

Mind that I'm using this.log (instead of this.logger). AFAIK, this.log is already scoped to http.server, calling this.log.get calls a nested logger. If we ever change the parent, it all changes together (as opposed to using the root logger this.logger). I hope it makes sense.

Personally, I'd prefer being able to filter logs by a full logger name that keeps some sort of record of where it's coming from.

++ I agree.

Copy link
Contributor Author

@TinaHeiligers TinaHeiligers Mar 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

using this.log (instead of this.logger)

I did a quick test with this.log.get(this.name, 'options') with the same http-console` appender. We end up duplicating the server name:

HTTP--[2023-03-06T10:05:25.632-07:00][DEBUG][http.server.Kibana.Kibana.options]---access [internal] for path [/internal/apm/service-map/service/{serviceName}]

Copy link
Member

@afharo afharo Mar 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh! cool! I didn't notice the existing entry is logged as http.server.Kibana. In that case, this.log.get('options') is enough :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done: commit fb86412 (#152621)

this.log.debug(`registering route handler for [${route.path}]`);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if extending this log entry with the values suggested in https://github.com/elastic/kibana/pull/152621/files/bf7a3c36cb4394b06a3b3fa09a0553c3f958bfed#r1126120651 would suffice. Both entries look too similar and are logged at the same time. If the only reason is so that we can use a new logger, I wonder if we can use it here (as long as it's nested, it should still log with the same config)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought about that but went a new logger route to be able to silence the extra info or to later remove the logger completely without losing the original log

// Hapi does not allow payload validation to be specified for 'head' or 'get' requests
const validate = isSafeMethod(route.method) ? undefined : { payload: true };
Expand All @@ -526,6 +527,8 @@ export class HttpServer {
xsrfRequired: route.options.xsrfRequired ?? !isSafeMethod(route.method),
access: route.options.access ?? (route.path.startsWith('/internal') ? 'internal' : 'public'),
};
// Log HTTP API target consumer.
optionsLogger.debug(`access [${kibanaRouteOptions.access}] for path [${route.path}]`);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mind that @jloleysens's comment also suggested adding the method

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apologies, adding that now.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done: commit fb86412


this.server!.route({
handler: route.handler,
Expand Down