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

Add ECS support #91

Merged
merged 12 commits into from
Feb 4, 2024
Merged

Add ECS support #91

merged 12 commits into from
Feb 4, 2024

Conversation

andriilahuta
Copy link
Contributor

This is a follow up to #90.
I'm not sure if that's the right approach for it to be merged into mainstream, so I'd appreciate your input @eht16.
Fixes #39.

Copy link
Owner

@eht16 eht16 left a comment

Choose a reason for hiding this comment

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

Wow, looks great.

And 💯 for the extensive tests.

I tested the code with the legacy formatter and it works fine.

With the ECS formatter, the final Logstash event has JSON fields like "log.syslog.hostname", will this be stored as object structure in ES or as is?

logstash_async/formatter.py Outdated Show resolved Hide resolved
logstash_async/formatter.py Outdated Show resolved Hide resolved
logstash_async/formatter.py Show resolved Hide resolved
@andriilahuta
Copy link
Contributor Author

andriilahuta commented Jan 4, 2024

With the ECS formatter, the final Logstash event has JSON fields like "log.syslog.hostname", will this be stored as object structure in ES or as is?

It seems to get mapped as an object structure.
Although it is apparently recommended to send nested objects instead.
So, I think it probably makes sense to dedot those keys similarly to how it's done in their formatter implementation.
WDYT?

@eht16
Copy link
Owner

eht16 commented Jan 7, 2024

With the ECS formatter, the final Logstash event has JSON fields like "log.syslog.hostname", will this be stored as object structure in ES or as is?

It seems to get mapped as an object structure. Although it is apparently recommended to send nested objects instead. So, I think it probably makes sense to dedot those keys similarly to how it's done in their formatter implementation. WDYT?

Having a consistent, ideally dedotted structure on the created events sounds good.

But please be aware that we cannot use the mentioned implementation here as it is licensed as "Apache License, Version 2.0" or even "Elasticsearch B.V." (I don't understand their licensing anymore :( ). This library is licensed as MIT and so we cannot use "Apache License, Version 2.0" licensed code.

Either you find an implementation with a compatible license or write it from scratch.

@andriilahuta
Copy link
Contributor Author

We really only need merge_dicts and de_dot functions from there which are generic enough and I had a slightly different implementation for them in mind anyway.

@eht16
Copy link
Owner

eht16 commented Jan 7, 2024

Alright, then go ahead.

@eht16
Copy link
Owner

eht16 commented Jan 21, 2024

The changes look good and work as expected.
Great job!

There are some linting issues, after those get fixed, I think we are ready to merge.

logstash_async/constants.py:47:101: E501 line too long (116 > 100 characters)
logstash_async/formatter.py:268:101: E501 line too long (105 > 100 characters)
logstash_async/formatter.py:450:101: E501 line too long (103 > 100 characters)
tests/formatter_test.py:16:101: E501 line too long (106 > 100 characters)

@andriilahuta
Copy link
Contributor Author

Linting issues should be fixed now.

@eht16
Copy link
Owner

eht16 commented Jan 28, 2024

I fixed the PyLint warnings:

Command line or configuration file:1: [E0013(bad-plugin-value), ] Plugin 'pylint.extensions.comparetozero' is impossible to load, is it installed ? ('No module named 'pylint.extensions.comparetozero'')
Command line or configuration file:1: [E0013(bad-plugin-value), ] Plugin 'pylint.extensions.emptystring' is impossible to load, is it installed ? ('No module named 'pylint.extensions.emptystring'')

in master.
Feel free to rebase your changes onto it or just ignore them.

The Flask warning about the version can be easily fixed:

import importlib.metadata
...
importlib.metadata.version("flask")

if you like to add it, otherwise I will do it after this one is merged.

But the tests seem to have various problems on older Python versions and other problems on recent Python versions :(.
Do you know you can easily run them locally with tox?

And I don't know why I have to approve every single CI run after your pushes. Basically it should run automatically after approved once, I thought.

@andriilahuta
Copy link
Contributor Author

Tests should work now.

And I don't know why I have to approve every single CI run after your pushes. Basically it should run automatically after approved once, I thought.

I thought so as well.
Docs mention this (https://docs.github.com/en/actions/managing-workflow-runs/approving-workflow-runs-from-public-forks):

Workflows on pull requests to public repositories from some outside contributors will not run automatically, and might need to be approved first. By default, all first-time contributors require approval to run workflows.

The wording is a bit vague regarding subsequent approvals though.

@eht16
Copy link
Owner

eht16 commented Feb 4, 2024

Nice that you added Python 3.12 to the tests!

Thank you for your efforts and the great work!

@eht16 eht16 merged commit baf2118 into eht16:master Feb 4, 2024
5 checks passed
@eht16
Copy link
Owner

eht16 commented Feb 4, 2024

I will add some docs and make a release next week.

@andriilahuta andriilahuta deleted the ecs branch February 4, 2024 22:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Broken host mapping for ECS
2 participants