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 Journald integration to input type #5890

Merged
merged 6 commits into from
Apr 25, 2023

Conversation

chemamartinez
Copy link
Contributor

@chemamartinez chemamartinez commented Apr 17, 2023

What does this PR do?

Convert the Journald integration to an input type package according to the specs located here.

It has also been moved to GA by bumping its version to 1.0.0.

Checklist

  • I have reviewed tips for building integrations and this pull request is aligned with them.
  • I have verified that all data streams collect metrics or logs.
  • I have added an entry to my package's changelog.yml file.
  • I have verified that Kibana version constraints are current according to guidelines.

Related issues

Screenshots

image

@chemamartinez chemamartinez added enhancement New feature or request Integration:journald Custom Journald logs labels Apr 17, 2023
@elasticmachine
Copy link

elasticmachine commented Apr 17, 2023

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview preview

Expand to view the summary

Build stats

  • Start Time: 2023-04-20T15:52:51.271+0000

  • Duration: 14 min 55 sec

Test stats 🧪

Test Results
Failed 0
Passed 1
Skipped 1
Total 2

🤖 GitHub comments

Expand to view the GitHub comments

To re-run your PR in the CI, just comment with:

  • /test : Re-trigger the build.

@P1llus
Copy link
Member

P1llus commented Apr 17, 2023

@jsoriano @hop-dev I think we need to quickly disucss the current spec/format for input packages before we merge this one. I want to see if we can clarify a few things we have issues with.

  1. Is the format for system test folders the same now without datastreams?
  2. How do we generate docs?
  3. Where should we put build.yml?

Looking at the log integration package, it is missing several crucial pieces, so we cant use that as a reference.

I thought it would be quickest to discuss this quickly on slack, so we can get a good understanding of the intended format.

@jsoriano
Copy link
Member

@jsoriano @hop-dev I think we need to quickly disucss the current spec/format for input packages before we merge this one. I want to see if we can clarify a few things we have issues with.

  1. Is the format for system test folders the same now without datastreams?

It is mostly the same, yes. Regarding the spec intended format, you can see an input package as a single datastream.

  1. How do we generate docs?

In principle it is also the same, with files under the docs directory. Though current helpers for data streams probably won't work now. I think we haven't added any input with documented fields yet, this would be something to improve if it doesn't work now.

  1. Where should we put build.yml?

According to the spec, in the same place as in integration packages, in _dev/build/build.yml.

Looking at the log integration package, it is missing several crucial pieces, so we cant use that as a reference.

Are you missing other pieces apart of the ones you mention here?

I thought it would be quickest to discuss this quickly on slack, so we can get a good understanding of the intended format.

Maybe we can schedule a meeting, including also @hop-dev and @ishleenk17.

Copy link
Contributor

@ishleenk17 ishleenk17 left a comment

Choose a reason for hiding this comment

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

Datastream name needs to be taken up as input from the user in your journald.yml.hbs file

Copy link
Contributor

@ishleenk17 ishleenk17 left a comment

Choose a reason for hiding this comment

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

For input packages, we let the user do all the fields mappings through Kibana.
We don't do any mapping of the processed data under fields (like done in input.yml).
Only very generic fields should be mapped here.

@ishleenk17
Copy link
Contributor

ishleenk17 commented Apr 19, 2023

2. How do we generate docs?

We don't get the sample event in the docs using the helpers, unlike in integrations.
It doesn't follow the auto generations format. It has to be done manually.

These are some of the input packages already implemented:
Jolokia Input, SQL, Statsd.

All of them might not have system tests though.

@ishleenk17
Copy link
Contributor

ishleenk17 commented Apr 19, 2023

Maybe we can schedule a meeting, including also @hop-dev and @ishleenk17.

Happy to do that.
I have shared some of my initial review comments. Let me know if a meeting is required as well.

@P1llus
Copy link
Member

P1llus commented Apr 19, 2023

@ishleenk17

Datastream name needs to be taken up as input from the user in your journald.yml.hbs file

Shouldn't we let elastic-package or fleet handle this? We already inject the UI elements to configure this, might as well also inject it into the config? Its easy to forget, because we have to remove the datastream settings from the manifest.yml, but keep it in the *.yml.hbs files.

@ishleenk17
Copy link
Contributor

@ishleenk17

Datastream name needs to be taken up as input from the user in your journald.yml.hbs file

Shouldn't we let elastic-package or fleet handle this? We already inject the UI elements to configure this, might as well also inject it into the config? Its easy to forget, because we have to remove the datastream settings from the manifest.yml, but keep it in the *.yml.hbs files.

Fleet might be the right place to handle this for the input package.
But, currently, it's not supported. So you might want to add it for now and raise this request with Fleet.

@chemamartinez
Copy link
Contributor Author

For input packages, we let the user do all the fields mappings through Kibana.
We don't do any mapping of the processed data under fields (like done in input.yml).
Only very generic fields should be mapped here.

@ishleenk17 I have some doubts regarding this suggestion. I understand we want let the users do the mapping of non-generic fields. However there are certain fields in input.yml that always appear in events (e.g. journald.pid) that might be interesting to keep.

@chemamartinez chemamartinez marked this pull request as ready for review April 20, 2023 11:29
@chemamartinez chemamartinez requested a review from a team as a code owner April 20, 2023 11:29
@elasticmachine
Copy link

Pinging @elastic/security-external-integrations (Team:Security-External Integrations)

Copy link
Member

@P1llus P1llus left a comment

Choose a reason for hiding this comment

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

LGTM. Let's do the planned manual tests first, then we can merge if its all working as intended.

@chemamartinez
Copy link
Contributor Author

Solved an issue with the mapping of event.created field.

Screenshot 2023-04-20 at 16 23 48

Fixed at 892a487

Copy link
Contributor

@pierrehilbert pierrehilbert left a comment

Choose a reason for hiding this comment

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

If you can just add the link to your E2E tests here it would be perfect!
Other than that, LGTM!

Copy link
Contributor

@ishleenk17 ishleenk17 left a comment

Choose a reason for hiding this comment

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

Looks good!

@chemamartinez chemamartinez merged commit 82c4c2e into elastic:main Apr 25, 2023
@elasticmachine
Copy link

Package journald - 1.0.0 containing this change is available at https://epr.elastic.co/search?package=journald

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request Integration:journald Custom Journald logs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Journald] Convert to input package and make GA
8 participants