-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[core.logging] Ensure LogMeta is ECS-compliant. #96350
Conversation
e75649e
to
371d043
Compare
Ecs, | ||
EcsEventCategory, | ||
EcsEventKind, | ||
EcsEventOutcome, | ||
EcsEventType, |
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 ended up exporting these from core because audit logging will use some of them.
@@ -35,9 +41,15 @@ const logStateTransition = ( | |||
newState: State | |||
) => { | |||
if (newState.logs.length > oldState.logs.length) { | |||
newState.logs | |||
.slice(oldState.logs.length) | |||
.forEach((log) => logger[log.level](logMessagePrefix + log.message)); |
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.
Spent awhile battling TS on this (and a similar situation in the actions
plugin).
The issue is that dynamically calling a method from Logger
results in an This expression is not callable
error due to each member of the union type having signatures which are incompatible (similar to this issue).
Specifically, it seems that when you are mixing methods which could contain string | Error
in the message (warn, error, fatal) with ones that only contain string
(trace, debug, info), TS freaks out.
But interestingly, this only surfaced when I added the generic type params to the Logger
interface... removing them resolves the issue.
Still not sure what's up with that, but would appreciate any insights folks may have. In the meantime, I resorted to casting in both places 😞
371d043
to
7cd8181
Compare
Pinging @mshustov @thomheymann for any initial thoughts on the approach here before I take it out of draft and ping a bunch of people 🙂 I added a few comments for discussion. |
7cd8181
to
19f202b
Compare
19f202b
to
2d63e5b
Compare
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.
Nice one, looks great Luke!
One suggestion regarding quarantined fields below.
packages/kbn-logging/src/logger.ts
Outdated
export interface LogMeta { | ||
[key: string]: any; | ||
} | ||
export type LogMeta = Partial<Omit<Ecs, 'ecs'>>; |
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.
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.
Let's a public interface with ecs field internally
The only issue I can see with this is a maintenance one -- if we update versions of ECS (or if someday the types are auto generated), then we will need to manually update the top-level keys in LogMeta
as well. However, since top-level keys don't change all that much, it probably wouldn't hurt to duplicate them for now, and reconsider later if it turns out to be a problem in practice. Will update.
category: ['database'], | ||
type: type ? [type] : undefined, |
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.
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.
Draft PR for the filebeat module: elastic/beats#25101
(@legrego @thomheymann would appreciate any expertise you could offer on that) ❤️
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.
Thanks for testing how this gets displayed. I think we might need to check with the Observability team if they support arrays for event.type
and event.category
and how that should be rendered.
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.
Looks great, really happy with the improvements!
Tested locally and audit logging is working as expected.
Got two questions. One below regarding ignore filters and the other one in the link regarding backwards compatibility: https://github.com/elastic/beats/pull/25101/files#r613936133
@elasticmachine merge upstream |
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.
alerting changes LGTM
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.
Audit logging changed LGTM! 👍
@elasticmachine merge upstream |
💚 Build SucceededMetrics [docs]Unknown metric groupsAPI count
API count missing comments
API count with any type
History
To update your PR or re-run it, just comment with: cc @lukeelmers |
💚 Backport successful
This backport PR will be merged automatically after passing CI. |
Co-authored-by: Luke Elmers <[email protected]>
Closes #90406
One goal of our new logging system is for the logs to be ECS-compliant (when using JSON layout). The one part of a log record that is outside of core's control is the
LogMeta
, where developers can put pretty much any object they want and have it added to the logs.While this provides nice flexibility, it poses a challenge when it comes to adhering to ECS formats: since we merge any
LogMeta
into the actual log record itself, it means that folks would ideally place theirmeta
into a known ECS field (if one exists for their data), or otherwise choose a custom field to use.In looking more closely at #90406, we decided that we would create a full set of ECS typings in core to use for promoting ECS-friendly
LogMeta
.This PR makes a few different changes to help with this effort:
@kbn/logging
, and also exports them from core.LogMeta
types to ensure they adhere to ECS types.Logger
so that folks can pass their own custom types which extend from thee baseLogMeta
/ECS types.LogMeta
in core/plugins.Plugin API Changes
Core's logging system has been updated to ensure that logs using a JSON layout are compliant with ECS. If you are using the optional
LogMeta
param in your plugin, you should check the ECS spec and ensure your meta conforms to ECS wherever possible.To assist with this, we've updated the
LogMeta
TypeScript interface to require ECS-friendly data. Should you need to log fields that do not fit within the ECS spec, you can provide a generic type parameter which accepts an interface that extends from the baseLogMeta
: