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 support for multi-core thread_id in filebeat postgresql module #9431

Closed
wants to merge 7 commits into from
Closed

Add support for multi-core thread_id in filebeat postgresql module #9431

wants to merge 7 commits into from

Conversation

kaiyan-sheng
Copy link
Contributor

@kaiyan-sheng kaiyan-sheng commented Dec 7, 2018

No description provided.

ph and others added 2 commits November 27, 2018 10:51
Remove default version qualifier and rename the environment variable to set it from `BEAT_VERSION_QUALIFIER` to `VERSION_QUALIFIER` this will align with other parts of the stack.

**Tested with filebeat.**
```
 ❯ ./filebeat version                                                                                                                                                                                                                                                                                                                                          [08:39:01]
filebeat version 7.0.0 (amd64), libbeat 7.0.0 [0a0c267 built 2018-11-19 13:38:15 +0000 UTC]
```

**Without the patch**
```
 ❯ ./filebeat version                                                                                                                                                                                                                                                                                                                                          [08:40:07]
filebeat version 7.0.0-alpha1 (amd64), libbeat 7.0.0-alpha1 [b007837 built 2018-11-19 13:39:59 +0000 UTC]
```

Fixes: #8384
@kaiyan-sheng
Copy link
Contributor Author

core_id is probably a bad name here. I am totally open to a better name!

@ruflin ruflin requested a review from sayden December 7, 2018 07:52
@ruflin ruflin added module review Filebeat Filebeat Team:Integrations Label for the Integrations team labels Dec 7, 2018
@elasticmachine
Copy link
Collaborator

Pinging @elastic/infrastructure

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.

This will also need a changelog entry

@@ -6,8 +6,6 @@
"input.type": "log",
"log.offset": 0,
"message": "2017-07-31 13:36:42.585 CEST [4974] LOG: database system was shut down at 2017-06-17 16:58:04 CEST",
"postgresql.log.level": "LOG",
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are these values missing now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I broke it 👎

@sayden
Copy link
Contributor

sayden commented Dec 7, 2018

I have seen that the input log lines can be very tricky so don't forget that you can try with more than one Grok patterns too.

@kaiyan-sheng
Copy link
Contributor Author

@ruflin yes for the changelog. Now I have this pr number, I can add it into the changelog :-)

@kaiyan-sheng kaiyan-sheng added in progress Pull request is currently in progress. and removed review labels Dec 7, 2018
@kaiyan-sheng
Copy link
Contributor Author

@sayden Good point! I just updated filebeat/module/postgresql/log/test/postgresql-9.5-multi-core.log with more log lines.

@@ -0,0 +1,1092 @@
[
Copy link
Member

Choose a reason for hiding this comment

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

No need to add this expected file, some longer test files are only to check that the content can be parsed, not to check the generated events.

"%{LOCALDATETIME:postgresql.log.timestamp} %{WORD:postgresql.log.timezone} \\[(%{NUMBER:postgresql.log.thread_id}|%{NUMBER:postgresql.log.thread_id}-%{BASE16FLOAT:postgresql.log.core_id})\\] \\[%{USERNAME:postgresql.log.user}\\]@\\[%{HOSTNAME:postgresql.log.database}\\] %{WORD:postgresql.log.level}: duration: %{NUMBER:postgresql.log.duration} ms statement: %{MULTILINEQUERY:postgresql.log.query}",
"%{LOCALDATETIME:postgresql.log.timestamp} %{WORD:postgresql.log.timezone} \\[(%{NUMBER:postgresql.log.thread_id}|%{NUMBER:postgresql.log.thread_id}-%{BASE16FLOAT:postgresql.log.core_id})\\] %{USERNAME:postgresql.log.user}@%{HOSTNAME:postgresql.log.database} %{WORD:postgresql.log.level}: ?%{GREEDYDATA:postgresql.log.message}",
"%{LOCALDATETIME:postgresql.log.timestamp} %{WORD:postgresql.log.timezone} \\[(%{NUMBER:postgresql.log.thread_id}|%{NUMBER:postgresql.log.thread_id}-%{BASE16FLOAT:postgresql.log.core_id})\\] \\[%{USERNAME:postgresql.log.user}\\]@\\[%{HOSTNAME:postgresql.log.database}\\] %{WORD:postgresql.log.level}: ?%{GREEDYDATA:postgresql.log.message}",
"%{LOCALDATETIME:postgresql.log.timestamp} %{WORD:postgresql.log.timezone} \\[(%{NUMBER:postgresql.log.thread_id}|%{NUMBER:postgresql.log.thread_id}-%{BASE16FLOAT:postgresql.log.core_id})\\] %{WORD:postgresql.log.level}: ?%{GREEDYDATA:postgresql.log.message}",
Copy link
Member

Choose a reason for hiding this comment

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

I think this is not related to the postgresql module, it is intended to test the fields generator, so these changes are probably not needed. But now that you have added them, should we add the core_id to the list of expected fields below?

@@ -6,7 +6,7 @@
"field": "message",
"ignore_missing": true,
"patterns": [
"^%{LOCALDATETIME:postgresql.log.timestamp} %{WORD:postgresql.log.timezone} \\[%{NUMBER:postgresql.log.thread_id}\\] ((\\[%{USERNAME:postgresql.log.user}\\]@\\[%{POSTGRESQL_DB_NAME:postgresql.log.database}\\]|%{USERNAME:postgresql.log.user}@%{POSTGRESQL_DB_NAME:postgresql.log.database}) )?%{WORD:postgresql.log.level}: (duration: %{NUMBER:postgresql.log.duration} ms statement: %{GREEDYDATA:postgresql.log.query}|%{GREEDYDATA:postgresql.log.message})"
"^%{LOCALDATETIME:postgresql.log.timestamp} %{WORD:postgresql.log.timezone} \\[(%{NUMBER:postgresql.log.thread_id}|%{NUMBER:postgresql.log.thread_id}-%{BASE16FLOAT:postgresql.log.core_id})\\] ((\\[%{USERNAME:postgresql.log.user}\\]@\\[%{POSTGRESQL_DB_NAME:postgresql.log.database}\\]|%{USERNAME:postgresql.log.user}@%{POSTGRESQL_DB_NAME:postgresql.log.database}) )?%{WORD:postgresql.log.level}: (duration: %{NUMBER:postgresql.log.duration} ms statement: %{GREEDYDATA:postgresql.log.query}|%{GREEDYDATA:postgresql.log.message})"
Copy link
Member

Choose a reason for hiding this comment

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

Could we add the core_id as optional so thread_id appears only one in the expression? i.e: \\[%{NUMBER:postgresql.log.thread_id}(-%{BASE16FLOAT:postgresql.log.core_id})?\\]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes! good point! Thanks! I destroyed this pr somehow ... I will create a new PR instead...

@kaiyan-sheng
Copy link
Contributor Author

Thanks for all the comments. I addressed them in #9482 and closing this one... I somehow managed to destroy it again...

@kaiyan-sheng kaiyan-sheng deleted the postgresql_multi_core branch December 11, 2018 00:03
@kaiyan-sheng kaiyan-sheng removed Team:Integrations Label for the Integrations team Filebeat Filebeat in progress Pull request is currently in progress. module labels Dec 11, 2018
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.

6 participants