Do not sort context tags in legacy logger #49530
Conversation
joshdover
added
Team:Core
|
Pinging @elastic/kibana-platform (Team:Platform) |
💔 Build Failed |
joshdover
changed the title
| const tags = _(data.tags) | ||
| .sortBy(function (tag) { | ||
| if (color(tag) === _.identity) return `2${tag}`; | ||
| if (_.includes(statuses, tag)) return `0${tag}`; | ||
| return `1${tag}`; | ||
| }) |
There was a problem hiding this comment.
So basically, every meta informations where put in the same tags list, and that's the way we were trying to reorder them the best we can ? That seems like a poor implementation.
However just to understand, the mechanism did not change between legacy and NP, so I'm not sure why we only encounter the issue in NP ? Is that because in legacy we basically didn't have multiple non-status tags, so that no more than one tag was sorted by return 1${tag}; ?
| const isColor = tag => color(tag) === _.identity; | ||
| const isStatus = tag => _.includes(statuses, tag); | ||
| const isContext = tag => tag.__kibanaContext__ === true; | ||
|
|
||
| const contextTags = data.tags.filter(isContext).map(tag => tag.value); | ||
| const nonContextTags = data.tags.filter(tag => !isContext(tag)); | ||
|
|
||
| const coloredTags = nonContextTags.filter(isColor).sort(); | ||
| const statusTags = nonContextTags.filter(isStatus).sort(); | ||
| const otherTags = nonContextTags.filter(tag => !isColor(tag) && !isStatus(tag) && !isContext(tag)).sort(); | ||
|
|
||
| const tags = [ | ||
| ...coloredTags, | ||
| ...statusTags, | ||
| ...contextTags, | ||
| ...otherTags, | ||
| ] |
There was a problem hiding this comment.
Follow to previous comment, but imho this is not the responsibility of a log format. What would be the amount of work to properly types the tags instead of having a flat list ?
Thinking of something like
type LogEntryTags = {
status: string;
context: string[];
// probably other things, don't know everything we did put in there
}That way, the formatter could apply a actual (even if still hardcoded) format instead of having some identification and sorting logic in there?
There was a problem hiding this comment.
👍 to evaluate how much work requires introducing a separate log record format for NP. Although I'd avoid putting this under tag property. This part in LP isn't covered by TS.
https://github.com/restrry/kibana/blob/b9a43a1788486c0d064cbe7aaff1fc7e987f7dfa/src/core/server/legacy/logging/legacy_logging_server.ts#L112-L116
There was a problem hiding this comment.
also we can ignore the problem and fix it when move logging infra to NP
There was a problem hiding this comment.
TBH, I'm leaning towards punting on this for now as well. It's not a new issue and we need to do the work to complete the migration to the new logger anyways, where we can fix this there.
|
Closing in favor of fixing this in the NP logger in #41956. |
Summary
Fixes #49524
Hacky way of making sure that context parts are not sorted by the legacy logger. Prevents breaking the log output. Opening PR for discussion on solution.
Still requires tests.
Checklist
Use
strikethroughsto remove checklist items you don't feel are applicable to this PR.For maintainers