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

[core.logging] Ensure LogMeta is ECS-compliant. #96350

Merged
merged 18 commits into from
Apr 20, 2021

Conversation

lukeelmers
Copy link
Member

@lukeelmers lukeelmers commented Apr 6, 2021

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 their meta 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:

  1. Adds full set of ECS typings (v1.9) to @kbn/logging, and also exports them from core.
  2. Updates LogMeta types to ensure they adhere to ECS types.
  3. Adds generic type params to Logger so that folks can pass their own custom types which extend from thee base LogMeta/ECS types.
  4. Updates downstream uses of LogMeta in core/plugins.
  5. Makes a few changes to security's audit logging, which already had its own ECS implementation that we can now align with core's.

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 base LogMeta:

// plugins/my-plugin/server/plugin.ts

import type { CoreSetup, LogMeta, Plugin, PluginInitializerContext } from 'src/core/server';

interface MyPluginLogMeta extends LogMeta {
  kibana: { myCustomField: string };
}

export class MyPlugin implements Plugin  {
  constructor(private readonly initContext: PluginInitializerContext) {
    this.logger = initContext.logger.get();
  }

  setup(core: CoreSetup) {
    this.logger.warn<MyPluginLogMeta>('my log with custom meta', {
      kibana: {
        myCustomField: 'heya',
      }
    });
  }
}

@lukeelmers lukeelmers self-assigned this Apr 6, 2021
@lukeelmers lukeelmers force-pushed the feat/ecs-logs branch 2 times, most recently from e75649e to 371d043 Compare April 12, 2021 04:50
packages/kbn-logging/src/logger.ts Show resolved Hide resolved
Comment on lines +239 to +243
Ecs,
EcsEventCategory,
EcsEventKind,
EcsEventOutcome,
EcsEventType,
Copy link
Member Author

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.

src/core/server/logging/layouts/json_layout.ts Outdated Show resolved Hide resolved
@@ -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));
Copy link
Member Author

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 😞

packages/kbn-logging/src/ecs/event.ts Show resolved Hide resolved
@lukeelmers
Copy link
Member Author

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.

@lukeelmers lukeelmers added release_note:plugin_api_changes Contains a Plugin API changes section for the breaking plugin API changes section. and removed WIP Work in progress labels Apr 12, 2021
Copy link
Contributor

@thomheymann thomheymann left a 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.

src/core/server/logging/layouts/json_layout.ts Outdated Show resolved Hide resolved
packages/kbn-logging/src/logger.ts Show resolved Hide resolved
packages/kbn-logging/src/ecs/agent.ts Show resolved Hide resolved
src/core/server/logging/layouts/json_layout.ts Outdated Show resolved Hide resolved
packages/kbn-logging/src/logger.ts Show resolved Hide resolved
export interface LogMeta {
[key: string]: any;
}
export type LogMeta = Partial<Omit<Ecs, 'ecs'>>;
Copy link
Contributor

Choose a reason for hiding this comment

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

what about avoiding Omit and Pick in the public interfaces? Let's a public interface with ecs field internally

the current error message :
2021-04-13_18-41-13

Copy link
Member Author

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.

@lukeelmers lukeelmers added the Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc label Apr 14, 2021
@lukeelmers lukeelmers marked this pull request as ready for review April 14, 2021 05:21
@lukeelmers lukeelmers requested review from a team as code owners April 14, 2021 05:21
Comment on lines +66 to +67
category: ['database'],
type: type ? [type] : undefined,
Copy link
Member Author

Choose a reason for hiding this comment

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

I tested the Kibana filebeat module and confirmed that converting this to an array, while correct from an ECS standpoint, will still break existing filters, etc:

Screen Shot 2021-04-14 at 3 39 56 PM

I'm working on an update to the module to correct this.

Copy link
Member Author

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) ❤️

Copy link
Contributor

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.

Copy link
Contributor

@thomheymann thomheymann left a 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

src/core/server/logging/layouts/json_layout.ts Outdated Show resolved Hide resolved
src/core/server/logging/layouts/json_layout.ts Outdated Show resolved Hide resolved
@lukeelmers
Copy link
Member Author

@elasticmachine merge upstream

Copy link
Member

@pmuellr pmuellr left a comment

Choose a reason for hiding this comment

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

alerting changes LGTM

Copy link
Contributor

@thomheymann thomheymann left a 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! 👍

@lukeelmers
Copy link
Member Author

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Unknown metric groups

API count

id before after diff
core 2174 2178 +4
security 79 71 -8
total -4

API count missing comments

id before after diff
core 992 995 +3
security 38 31 -7
total -4

API count with any type

id before after diff
core 147 146 -1

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

cc @lukeelmers

@lukeelmers lukeelmers added the auto-backport Deprecated - use backport:version if exact versions are needed label Apr 20, 2021
@lukeelmers lukeelmers merged commit 12b245c into elastic:master Apr 20, 2021
@lukeelmers lukeelmers deleted the feat/ecs-logs branch April 20, 2021 15:31
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Apr 20, 2021
@kibanamachine
Copy link
Contributor

💚 Backport successful

Status Branch Result
7.x

This backport PR will be merged automatically after passing CI.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-backport Deprecated - use backport:version if exact versions are needed Feature:Logging release_note:plugin_api_changes Contains a Plugin API changes section for the breaking plugin API changes section. Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v7.13.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Core] Logs in JSON layout follow ECS format
6 participants