Add Appender.receiveAllLevels option to fix LegacyAppender #55752
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
joshdover
added
Team:Core
|
Pinging @elastic/kibana-platform (Team:Platform) |
pgayvallet
approved these changes
Jan 24, 2020
joshdover
force-pushed
the
np/debug-logging-fix
branch
from
5944922
to
0e39b2c
Compare
|
Added tests. I was going to add some e2e tests but we don't have a great way of inspecting legacy logs without parsing stdout. In the past, these have been really brittle tests. Since this is somewhat of an edge case and this behavior will be removed in 8.x, I decided against adding yet another brittle test. |
joshdover
added
release_note:skip
pgayvallet
approved these changes
Jan 27, 2020
Comment on lines
414
to
425
| test('passes log record to appender with receiveAllLevels: true, regardless if log level is supported', () => { | ||
| const receiveAllAppender = { append: jest.fn(), receiveAllLevels: true }; | ||
| const warnLogger = new BaseLogger(context, LogLevel.Warn, [receiveAllAppender], factory); | ||
|
|
||
| warnLogger.trace('trace-message'); | ||
| expect(receiveAllAppender.append).toHaveBeenCalledTimes(1); | ||
| warnLogger.debug('debug-message'); |
There was a problem hiding this comment.
NIT:
- (probably covered in other tests, but) would probably check the call argument after each (or after last call of)
expect(receiveAllAppender.append).toHaveBeenCalledTimes(1); - Maybe an opposite test with
receiveAllLevels : false
joshdover
force-pushed
the
np/debug-logging-fix
branch
from
0e39b2c
to
8f7d322
Compare
💚 Build SucceededHistory
To update your PR or re-run it, just comment with: |
joshdover
added a commit
to joshdover/kibana
that referenced
this pull request
Jan 28, 2020
gmmorris
added a commit
to gmmorris/kibana
that referenced
this pull request
Jan 28, 2020
* master: (21 commits) [SIEM][Detection Engine] critical blocker updates to latest ECS version [Monitoring] Fix inaccuracies in logstash pipeline listing metrics (elastic#55868) Resetting errors and removing duplicates (elastic#56054) Add flag to opt out from sub url tracking (elastic#55672) [SIEM][Detection Engine] critical bug, fixes duplicate tags [ML] Anomaly Detection: Fix persist/restore of refreshInterval in globalState. (elastic#56113) [ML] Single Metric Viewer: Fix annnotations refresh. (elastic#56107) adapt ObjectToConfigAdapter.getFlattenedPaths to consider arrays as final values (elastic#56105) Add Appender.receiveAllLevels option to fix LegacyAppender (elastic#55752) [ML] Process delimited files like semi-structured text (elastic#56038) Charts plugin (combining ui/color_maps and EuiUtils) (elastic#55469) fix tutorial documentation (elastic#55996) [ML] Fix persist/restore of time/refreshInterval in data visualizer. (elastic#56122) [Index Management] Fix errors with validation (elastic#56072) [Index Management] Add try/catch when parsing index filter from URI (elastic#56051) [NP] add HTTP resources testing strategies (elastic#54908) [ML] Single Metric Viewer: Fix brush update on short recent timespans. (elastic#56125) [Uptime] Add timeout for slow process to skipped functional tests (elastic#56065) refactor (elastic#56121) Move tests in dashboard into appropriate folders (elastic#55304) ...
joshdover
added a commit
that referenced
this pull request
Jan 28, 2020
3 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #55456
The legacy logger & logging configuration work on the concept of log "tags" where each log entry can have a number of tags. In legacy, log levels are not treated special and are really just a tag.
In legacy, this allows logging configurations like:
Any logs that match any of the tags in this array will be logged. This allows the Kibana admin to add the "testbed" tag to the configuration, and get all "testbed" logs regardless of the log level.
In the Platform logger, we filter all logs based on the configured log level, before sending logs to each appender. Since the legacy logger is integrated as a appender in the Platform logger, all Platform logs will be filtered by log level before they can be processed by the legacy logger. This means a configuration like the one above will not forward logs at the
debuglevel to the legacy logger at all.This change adds a new
receiveAllLevelsproperty to theAppenderinterface that allows us to bypass this filtering for the LegacyAppender and allow the existing tag filtering mechanism in legacy to continue to work the same.I've opened this PR as a draft before writing tests to make sure we're ok with this temporary approach. Once #41956 is done, we may be able to fix this at the configuration level instead, transforming the legacy config into the appropriate Platform config to get the same behavior.
Checklist
Use
strikethroughsto remove checklist items you don't feel are applicable to this PR.For maintainers