-
Notifications
You must be signed in to change notification settings - Fork 8.1k
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
[browser logging] allow to configure root level #176397
[browser logging] allow to configure root level #176397
Conversation
/ci |
/ci |
/ci |
/ci |
/ci |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Self-review / comments
/** | ||
* @internal | ||
*/ | ||
export interface BrowserLoggingConfig { | ||
root: BrowserRootLoggerConfig; | ||
} | ||
|
||
/** | ||
* @internal | ||
*/ | ||
export interface BrowserRootLoggerConfig { | ||
level: LogLevelId; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Base types for the browser-side logging config. It's simplistic right now, but more fields should appear when we go further on #144276
@@ -63,6 +69,7 @@ export const config = { | |||
}), | |||
level: levelSchema, | |||
}), | |||
browser: browserConfig, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm exposing the browser logging configuration via the logging.browser
config prefix. So the browser-side config is "included" in the server-side logging prefix that is logging
.
I wasn't sure, but in the end I still think it makes sense to have a single prefix (and that it isn't worth introducing heavy changes to move the server-side logging under the logging.server
prefix either), but please tell if you think otherwise or have another idea.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be nice to have them symmetrical, but I think logging.browser.*
is fine
this.initContext.logger.debug<EventDebugLogMeta>(`Report event "${eventType}"`, { | ||
ebt_event: event, | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No longer needs this isDev
check now that the default level is info
and is configurable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
then again https://xkcd.com/1172
# Enables debug logging on the browser (dev console) | ||
#logging.browser.root: | ||
# level: debug |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure it was really needed given right now it's mostly for internal/development usage, but I still added a snippet in the config file.
Pinging @elastic/kibana-core (Team:Core) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did not test locally, code LGTM!
this.initContext.logger.debug<EventDebugLogMeta>(`Report event "${eventType}"`, { | ||
ebt_event: event, | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
then again https://xkcd.com/1172
@@ -63,6 +69,7 @@ export const config = { | |||
}), | |||
level: levelSchema, | |||
}), | |||
browser: browserConfig, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be nice to have them symmetrical, but I think logging.browser.*
is fine
💚 Build Succeeded
Metrics [docs]Page load bundle
History
To update your PR or re-run it, just comment with: |
## Summary Part of elastic#144276 - Introduce the concept of browser-side logging configuration, via a `logging.browser` config prefix - Allow to configure the log level for the root browser logger via `logging.browser.root.level` - Set the default level to `info` for both dev and production mode (consistent with server-side logging) --------- Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
## Summary Part of elastic#144276 - Introduce the concept of browser-side logging configuration, via a `logging.browser` config prefix - Allow to configure the log level for the root browser logger via `logging.browser.root.level` - Set the default level to `info` for both dev and production mode (consistent with server-side logging) --------- Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
## Summary Part of elastic#144276 - Introduce the concept of browser-side logging configuration, via a `logging.browser` config prefix - Allow to configure the log level for the root browser logger via `logging.browser.root.level` - Set the default level to `info` for both dev and production mode (consistent with server-side logging) --------- Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
Summary
Part of #144276
logging.browser
config prefixlogging.browser.root.level
info
for both dev and production mode (consistent with server-side logging)