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

Convert Filebeat kibana.log to ECS #9301

Merged
merged 12 commits into from
Jan 11, 2019
Merged

Conversation

webmat
Copy link
Contributor

@webmat webmat commented Nov 29, 2018

Caveats

One thing I think should be addressed by this PR:

  • I think we should append kibana.log.tags to tags. However what's the best way to approach it? append expects a JSON array of values or field names, not a field name that contains an array (see doc example)
    • Decision: Keeping under kibana. field set.

Actual caveats / disclaimers

Not previously done by this module. I say we should not add this now, as it adds dependencies:

  • Perform user agent parsing
  • Perform geoip on source.ip

Renames

  • kibana.log.meta.req.headers.referer => http.request.referrer
  • kibana.log.meta.req.headers.user-agent => user_agent.original
  • kibana.log.meta.req.remoteAddress => source.address
  • kibana.log.meta.req.url => url.original

Duplicate fields

They have been removed, are aliased as well

  • kibana.log.meta.req.referer => http.request.referrer
  • kibana.log.meta.statusCode => http.response.status_code (ECS field was already populated)
  • kibana.log.meta.method => http.request.method

TODO

@ruflin ruflin mentioned this pull request Nov 29, 2018
@webmat webmat self-assigned this Nov 29, 2018
@webmat webmat added in progress Pull request is currently in progress. module Filebeat Filebeat ecs labels Nov 29, 2018
@ruflin
Copy link
Member

ruflin commented Dec 3, 2018

Caveats:

  • service.name: I remember it has to do on how we add service.name. From a lucene perspective it's the same so not need to change. But there is an issue. This should become service.type as service.name can be a user configured name.
  • We should open an issue in the KB repo to start a discussion around ECS logs
  • Missing fields: Let's open track this during the discussion related to http fields

@webmat webmat requested a review from a team as a code owner December 20, 2018 21:34
@webmat webmat added review and removed in progress Pull request is currently in progress. labels Dec 20, 2018
@webmat webmat changed the title WIP Convert Filebeat kibana.log to ECS Convert Filebeat kibana.log to ECS Dec 20, 2018
@webmat webmat requested a review from ruflin December 20, 2018 21:38
@webmat
Copy link
Contributor Author

webmat commented Dec 20, 2018

@ruflin Apart from one thing that I wonder about (kibana.log.tags >> tags?), this should be pretty much ready to go.

@webmat
Copy link
Contributor Author

webmat commented Dec 21, 2018

jenkins, test this

Copy link
Member

@ruflin ruflin left a comment

Choose a reason for hiding this comment

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

If I understand this correctly, the fields which were removed and were duplicates, still have an alias so old stuff would keep working?

@ruflin
Copy link
Member

ruflin commented Dec 21, 2018

@webmat I'm good with keep kibana.logs.tags as I think these are tags specific to kibana.

@webmat
Copy link
Contributor Author

webmat commented Dec 21, 2018

@ruflin This is correct. Some fields were present twice in the output format, under different names. Now they are present only once, in line with ECS, and all the old names are aliases to the ECS fields.

@webmat
Copy link
Contributor Author

webmat commented Dec 21, 2018

@ruflin One more caveat I'd like your opinion on: service.name, we leave as an array of one?

@ruflin
Copy link
Member

ruflin commented Dec 21, 2018

@webmat Yes, for the array. It does not make a difference. Can explain an other time why it's like.

After we done all the modules, will go through an rename service.name to service.type anyways.

@webmat
Copy link
Contributor Author

webmat commented Dec 21, 2018

@ruflin Rebasing. Good to go, once tests are green?

@webmat
Copy link
Contributor Author

webmat commented Jan 3, 2019

@ruflin You had approved this before the holidays, I tweaked one small detail. Instead of renaming source.address to source.ip, I copy it (keeping source.address).

Can I get one last look, please? Thanks

@urso urso removed the request for review from a team January 4, 2019 01:57
@webmat webmat removed the blocked label Jan 4, 2019
@webmat
Copy link
Contributor Author

webmat commented Jan 7, 2019

Ping @elastic/stack-monitoring for awareness. If this works for you as well, I'd like to merge on Tuesday at the latest.

@webmat
Copy link
Contributor Author

webmat commented Jan 8, 2019

@ycombinator Hey could you have a quick look at this one as well, when you have a moment, please? Thanks!

"lang": "painless",
"source": "ctx.event.duration = Math.round(ctx.kibana.log.meta.res.responseTime * params.scale)",
"params": { "scale": 1000000 },
"if": "ctx.kibana.log.containsKey('meta') && ctx.kibana.log.meta.containsKey('res') && ctx.kibana.log.meta.res.containsKey('responseTime')"
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: You could probably shorten this to something like: ctx.kibana.log.meta?.res?.responseTime != null.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh man, Painless has that? Thanks for pointing out ❤️

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

@ycombinator Do you know which version this was added? Asking for compatibility reasons.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Ah, then we are good :-)

Copy link
Contributor

@ycombinator ycombinator left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for explaining the various changes to me off-PR. Left a nit comment that can feel free to ignore.

@ruflin
Copy link
Member

ruflin commented Jan 10, 2019

jenkins, test this

@webmat
Copy link
Contributor Author

webmat commented Jan 10, 2019

Rebased and updated the pipeline to use the null-safe operators. Thanks @ycombinator for pointing those out, this will make my life much easier in places like this experimental PR: #9905 😆

Tests were green across the board. Will merge tomorrow AM if it's still the case.

@webmat webmat removed the review label Jan 11, 2019
@webmat webmat merged commit 6c1d73b into elastic:master Jan 11, 2019
@webmat webmat deleted the ecs-kibana-fb branch January 11, 2019 02:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants