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 system.syslog to ECS #9135

Merged
merged 8 commits into from
Nov 22, 2018
Merged

Conversation

webmat
Copy link
Contributor

@webmat webmat commented Nov 16, 2018

Caveats

  • Syslog messages could be received from hosts and devices. Currently setting host.hostname. What should we do with device.hostname?
  • ECS is missing a timezone field, but should have one. Currently leaving to beats.timezone.

ECS Fields temporarily defined locally

  • host.hostname, added to system/_meta/fields.yml

Renames

  • system.syslog.hostname => host.hostname
  • system.syslog.program => process.name
  • system.syslog.pid => process.pid
  • system.syslog.message => message

TODO

  • Coerce pid to an int
  • Update ECS-migration.yml file
  • Changelog
  • Delete system.syslog.timestamp
  • Create field aliases

Noticed

@ruflin ruflin mentioned this pull request Nov 16, 2018
@webmat webmat self-assigned this Nov 16, 2018
@webmat webmat added in progress Pull request is currently in progress. module Filebeat Filebeat ecs labels Nov 16, 2018
"system.syslog.program": "GoogleSoftwareUpdateAgent",
"message": "2016-12-13 11:35:28.421 GoogleSoftwareUpdateAgent[21412/0x700007399000] [lvl=2] -[KSUpdateEngine updateAllExceptProduct:] KSUpdateEngine updating all installed products, except:'com.google.Keystone'.",
"process.name": "GoogleSoftwareUpdateAgent",
"process.pid": "21412",
Copy link
Member

Choose a reason for hiding this comment

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

IIRC process.pid needs converted to number.

Copy link
Contributor

Choose a reason for hiding this comment

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

I have the same issue in an other PR. I need to investigate if during the testing the ECS templates are also properly loaded.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This PR has not changed the fact that the PID was never properly cast to an int.

It's a trivial thing to fix in the grok, however. I'd be ok with fixing that in places where I see these values improperly cast. Could this introduce a breaking change, though? Visualizations are not affected by the field in _source being a string or an int, so that part should be fine. But are there other things that could be affected by these values going from a string to an int?

Copy link
Contributor Author

@webmat webmat Nov 19, 2018

Choose a reason for hiding this comment

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

Actually haproxy/log has it the worst. It has a mix of int and strings, both for the PID and for the port. If this was a problem, we would have heard about it by now. I'll go around and do this small improvement on all of my open PRs.

Copy link
Contributor Author

@webmat webmat Nov 19, 2018

Choose a reason for hiding this comment

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

It's a rabbit hole, though. Now I notice that IIS response code and response times are not converted to ints...

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think it's a grok issue here. It's more of a template issue in the tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, all of these fields have proper long mappings. It really is as simple as adding :int at the end of the GROK expression, so that the value in the JSON document is a integer rather than a quoted string.

I've actually gone deep in the rabbit hole and changed all of the occurrences I saw in this PR and all of the other ones I'm working on at the moment.

@webmat webmat removed the in progress Pull request is currently in progress. label Nov 20, 2018
@webmat
Copy link
Contributor Author

webmat commented Nov 20, 2018

@ruflin Ready for review. Make sure to check out caveats. Sorry on this app I can’t add the “review“ tag 😂

@ruflin ruflin changed the title WIP Convert Filebeat system.syslog to ECS Convert Filebeat system.syslog to ECS Nov 20, 2018
@ruflin ruflin added the review label Nov 20, 2018
@ruflin
Copy link
Contributor

ruflin commented Nov 20, 2018

Modified title and assigned label ;-)

@ruflin
Copy link
Contributor

ruflin commented Nov 20, 2018

I think this one is ready to go. We can do further adjustments in follow up PR's.

Copy link
Contributor Author

@webmat webmat left a comment

Choose a reason for hiding this comment

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

Test failure was a random network hiccup. Will re-trigger the build by pushing the deletion of system.syslog.timestamp and rebasing. Should be good for merging after that.

dev-tools/ecs-migration.yml Show resolved Hide resolved
Copy link
Contributor

@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.

For the caveats:

  • Let's go with host.hostname for now. I don't think we can tell if it comes from a device or host
  • I'm good with keeping beat.timezone for now until we figured out things in ECS. Let's add this to a "todo" list somewhere so we don't forget
  • event.original: Lets handle this in Filebeat modules: keep raw message #8083 as proposed, not here.

@webmat
Copy link
Contributor Author

webmat commented Nov 21, 2018

@ruflin Trying to add field aliases towards their ECS equivalent seems not to make these fields into aliases in the mapping. It also does the wrong thing in the documentation generated.

Steps to confirming the mapping:

  1. Rebuild make update && make filebeat.test && make filebeat
  2. Clean up to be extra sure: DELETE test-filebeat-modules
  3. Run system/syslog tests INTEGRATION_TESTS=1 GENERATE=1 TESTING_FILEBEAT_MODULES=system nosetests tests/system/test_modules.py
  4. Check the mapping again GET test-filebeat-modules/_mapping

Result: I'm seeing all of the aliased fields as normal keyword fields, not field aliases. Do you know what could be causing this? Is there an issue with the way I specify my aliases: commit?

Also, check out how all of the system.syslog fields I'm aliasing get replaced directly with their ECS equivalent here, that doesn't seem ok.

My take would be to

  1. Do the declarative part anyway. All field declarations that should be aliases are made so right now, and we merge as is
  2. Open issue to get the aliases to make it to the mapping (unless I'm doing something wrong)
  3. Open issue to generate correct documentation for alias fields.
  • Perhaps should now simply say "deprecated field, please use X in the future. This od field name will remain as an alias until version 8, and then be removed" or something to that effect

@webmat
Copy link
Contributor Author

webmat commented Nov 22, 2018

Only failure appears to be with this flaky test: #9207

Copy link
Contributor

@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.

I opened a PR to skip the failing test. Ready to be merged.

Mathieu Martin added 5 commits November 22, 2018 13:49
- system.syslog.hostname => host.hostname
- system.syslog.program => process.name
- system.syslog.pid => process.pid
- system.syslog.message => message
@webmat webmat merged commit ff83dcf into elastic:master Nov 22, 2018
@webmat webmat deleted the ecs-system-syslog branch November 22, 2018 18:58
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