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.

Sign up for GitHub

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

Jump to bottom

Update PostgreSQL integration to support logs in CSV format #747

Merged
merged 33 commits into from
Mar 22, 2021
Merged

Update PostgreSQL integration to support logs in CSV format #747

merged 33 commits into from
Mar 22, 2021

Conversation

jsoriano
Copy link
Member

@jsoriano jsoriano commented Feb 25, 2021 •
edited
Loading

What does this PR do?

Import latest changes in Beats. Including support for logs in CSV format (see #720, elastic/beats#23334).

  • Updated packages/postgresql/_dev/build/docs/README.md.
  • PACKAGES=postgresql mage ImportBeats.
  • Some manual grooming as recovering events in the README.
  • Copy test files from Beats.
  • Add docker deployment and system tests.
  • Add missing ECS fields to make tests pass.

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.

Author's Checklist

  • [ ]

How to test this PR locally

Related issues

Screenshots

@jsoriano jsoriano self-assigned this Feb 25, 2021
@elasticmachine
Copy link

elasticmachine commented Feb 25, 2021 •
edited
Loading

💚 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

Expand to view the summary

Build stats

  • Build Cause: Pull request #747 updated

  • Start Time: 2021-03-16T19:12:16.716+0000

  • Duration: 12 min 46 sec

  • Commit: 3eaee22

Test stats 🧪

Test Results
Failed 0
Passed 58
Skipped 0
Total 58

Trends 🧪

Image of Build Times

Image of Tests

@andresrc andresrc added Integration:postgresql PostgreSQL Team:Integrations Label for the Integrations team labels Feb 25, 2021
@jsoriano jsoriano mentioned this pull request Mar 2, 2021
Closed
@jsoriano jsoriano mentioned this pull request Mar 4, 2021
Merged
6 tasks
@jsoriano
Copy link
Member Author

jsoriano commented Mar 4, 2021

Failing test is caused by elastic/beats#24359

This was referenced Mar 5, 2021
Merged
Merged
@jsoriano jsoriano marked this pull request as ready for review March 5, 2021 14:36
@elasticmachine
Copy link

Pinging @elastic/integrations (Team:Integrations)

@jsoriano
Copy link
Member Author

jsoriano commented Mar 5, 2021

Opening for review, elastic/beats#24359 has been merged and should solve the failing test with next snapshots.

@jsoriano
Copy link
Member Author

jsoriano commented Mar 9, 2021

/test

mtojek
mtojek reviewed Mar 11, 2021
View reviewed changes
Copy link
Contributor

@mtojek mtojek left a comment

Choose a reason for hiding this comment

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

Nice improvement for PostgreSQL!

BTW You can rebase against master to run tests only for this integration (I pushed a fix yesterday).

packages/postgresql/data_stream/log/manifest.yml Outdated Show resolved Hide resolved
packages/postgresql/data_stream/statement/fields/ecs.yml
description: 'Name of the module this data is coming from.

If your monitoring agent supports the concept of modules or plugins to process events of a given source (e.g. Apache logs), `event.module` should contain the name of this module.'
example: apache
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I used to leave name, type and description in most cases (to shrink files), but you can leave all properties if you prefer.

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 copied directly from the ecs files in beats, but I will review it.

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 am leaving it as is, not sure what to remove.

packages/postgresql/manifest.yml Outdated Show resolved Hide resolved
packages/postgresql/manifest.yml Outdated Show resolved Hide resolved
@jsoriano
Copy link
Member Author

While regenerating dashboards I have seen that some log lines are not being correctly parsed, taking a look.

@jsoriano jsoriano mentioned this pull request Mar 11, 2021
Closed
@jsoriano
Copy link
Member Author

jsoriano commented Mar 16, 2021 •
edited
Loading

While regenerating dashboards I have seen that some log lines are not being correctly parsed, taking a look.

I hadn't configured PostgreSQL logs as Filebeat expects. There is an open issue about improving docs for this: elastic/beats#14337. More details about my investigation on this here: elastic/beats#20321 (comment)

There are a couple of things we could improve to avoid these issues, but I'd like to have this merged before.

@mtojek I think this would be ready for another review. Thanks!

kaiyan-sheng
kaiyan-sheng reviewed Mar 22, 2021
View reviewed changes
packages/postgresql/data_stream/bgwriter/fields/ecs.yml
required: true
type: keyword
ignore_above: 1024
description: 'ECS version this event conforms to. `ecs.version` is a required field and must exist in all events.
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: do we need the quotes here for all descriptions?

Copy link
Member Author

Choose a reason for hiding this comment

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

If they are multi-lined descriptions they need to be between quotes, or with yaml blocks started with | or |-.
I think elastic-package format fixed some of them, but I see we have all the options along these fields, so not sure which one to use, I would leave it as is by now.

@mtojek mtojek self-requested a review March 22, 2021 13:09
mtojek
mtojek approved these changes Mar 22, 2021
View reviewed changes
Copy link
Contributor

@mtojek mtojek left a comment

Choose a reason for hiding this comment

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

Ship it! Thank you for all adjustments.

@jsoriano jsoriano merged commit ce930df into elastic:master Mar 22, 2021
@jsoriano jsoriano deleted the postgresql-7-12 branch March 22, 2021 14:05
eyalkraft pushed a commit to build-security/integrations that referenced this pull request Mar 30, 2022
@jsoriano
Update PostgreSQL integration to support logs in CSV format (elastic#747
1307904 
)

Import latest changes in Beats. Including support for logs in CSV format.
* Import pipelines and test files from Beats.
* Update README.
* Add docker deployment and system tests.
* Add missing ECS fields to make tests pass.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kaiyan-sheng kaiyan-sheng kaiyan-sheng approved these changes

@mtojek mtojek mtojek approved these changes

Assignees

@jsoriano jsoriano

Labels
Integration:postgresql PostgreSQL Team:Integrations Label for the Integrations team
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add CSV logs support to postgresql integration
5 participants
@jsoriano @elasticmachine @mtojek @kaiyan-sheng @andresrc