Generic SQL join results of multiple queries by lalit-satapathy · Pull Request #32394 · elastic/beats

lalit-satapathy · 2022-07-19T03:44:27Z

What does this PR do?

This PR enable creating a single event for cases, where multiple individual queries are executed by proving a new flag.

Why is it important?

This PR enables single event generation that is needed by integration packages like mssql.

Pending

Update default sql.yml file

Checklist

My code follows the style guidelines of this project
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
[TBD] I have made corresponding change to the default configuration files
I have added tests that prove my fix is effective or that my feature works
I have added an entry in CHANGELOG.next.asciidoc or CHANGELOG-developer.next.asciidoc.

Author's Checklist

Integration test updated for multiple sql_queries.

How to test this PR locally

host any database instance like postgresql.
Run metricbeat by enabling request_merge with multiple sql_queries
Review the generated metrics

Related issues

Closes Generic SQL join results of multiple queries - discussion draft #31806

* Add flag request_merge to request merge for sql_queries. * Collect metrics when request_merge is enabled and report collectively at the end. * Flag error/warning when conditions not met.

Fixed integration to add merged mode. Fixed earlier integration test issues.

mergify · 2022-07-19T03:44:45Z

This pull request is now in conflicts. Could you fix it? 🙏
To fixup this pull request, you can check out it locally. See documentation: https://help.github.com/articles/checking-out-pull-requests-locally/

git fetch upstream
git checkout -b generic-sql-event-merge-1 upstream/generic-sql-event-merge-1
git merge upstream/main
git push upstream generic-sql-event-merge-1

elasticmachine · 2022-07-19T08:55:49Z

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS

Expand to view the summary

Build stats

Start Time: 2022-07-21T14:03:35.261+0000
Duration: 96 min 59 sec

Test stats 🧪

Test	Results
Failed	0
Passed	3875
Skipped	993
Total	4868

💚 Flaky test report

Tests succeeded.

🤖 GitHub comments

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

/test : Re-trigger the build.
/package : Generate the packages and run the E2E tests.
/beats-tester : Run the installation tests with beats-tester.
run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

mergify · 2022-07-20T09:52:26Z

This pull request is now in conflicts. Could you fix it? 🙏
To fixup this pull request, you can check out it locally. See documentation: https://help.github.com/articles/checking-out-pull-requests-locally/

git fetch upstream
git checkout -b generic-sql-event-merge-1 upstream/generic-sql-event-merge-1
git merge upstream/main
git push upstream generic-sql-event-merge-1

jsoriano · 2022-07-21T05:11:16Z

x-pack/metricbeat/module/sql/_meta/docs.asciidoc

+  request_merge: true
+  sql_queries:
+    - query: "SELECT blks_hit FROM pg_stat_database limit 1;"
+      response_format: table
+    - query: "SELECT blks_read FROM pg_stat_database limit 1;"
+      response_format: table


How would this configuration be different to the following one?

Suggested change

request_merge: true

sql_queries:

- query: "SELECT blks_hit FROM pg_stat_database limit 1;"

response_format: table

- query: "SELECT blks_read FROM pg_stat_database limit 1;"

response_format: table

sql_queries:

- query: "SELECT blks_hit, blks_read FROM pg_stat_database limit 1;"

response_format: table

If this would generate the same result, please put here an example of a case where request_merge would be the only option. I guess that this merge is required in cases where data from multiple tables is being collected.

Updated the example with example queries from two different tables.

jsoriano · 2022-07-21T05:17:26Z

x-pack/metricbeat/module/sql/query/query_integration_test.go

 				},
 			}

+			// Need to add this.


Why is it needed? I'd assume that any code we add is "needed" 🙂

If this test requires any explanation, please comment it here. If it doesn't, please remove this comment.

jsoriano · 2022-07-21T05:18:52Z

x-pack/metricbeat/module/sql/query/query_integration_test.go

+				},
+			}
+
+			// Need to add this.


Also here, please explain why this is needed, or remove the comment if no explanation is needed.

jsoriano · 2022-07-21T05:20:41Z

x-pack/metricbeat/module/sql/query/query_integration_test.go

+			cfg = testFetchConfig{
+				config: config{
+					Driver:         "postgres",
+					Queries:        []query{query{Query: "SELECT blks_hit FROM pg_stat_database limit 1;", ResponseFormat: "table"}, query{Query: "SELECT blks_read FROM pg_stat_database limit 1;", ResponseFormat: "table"}},


Nit. For readability please split this in multiple lines. Something like this at least:

Suggested change

Queries: []query{query{Query: "SELECT blks_hit FROM pg_stat_database limit 1;", ResponseFormat: "table"}, query{Query: "SELECT blks_read FROM pg_stat_database limit 1;", ResponseFormat: "table"}},

Queries: []query{

query{Query: "SELECT blks_hit FROM pg_stat_database limit 1;", ResponseFormat: "table"},

query{Query: "SELECT blks_read FROM pg_stat_database limit 1;", ResponseFormat: "table"},

},

Re-formatted.

jsoriano · 2022-07-21T05:25:21Z

x-pack/metricbeat/module/sql/query/query.go


-	Queries []query `config:"sql_queries" `
+	Queries      []query `config:"sql_queries" `
+	MergeQueries bool    `config:"request_merge"`


As commented in #31806 (comment), I don't like so much the name of request_merge for this option, it is not very meaningful. What are you merging? Is this a request that may not be fulfiled?

I would suggest something like merge_results, merge_events or single_event.

Suggested change

MergeQueries bool `config:"request_merge"`

MergeResults bool `config:"merge_results"`

Suggested change

MergeQueries bool `config:"request_merge"`

SingleEvent bool `config:"single_event"`

Changed the flag name as merge_results and also the variable. merge_results sounds fine with me, so let's go ahead with it.

jsoriano · 2022-07-21T05:37:37Z

x-pack/metricbeat/module/sql/query/query.go

 	}
+	if m.Config.MergeQueries {
+		// Report here for merged case.
+		m.reportEvent(merged, reporter, "MULTIPLE")


I think I'd prefer not to set sql.query on this case. Maybe you can pass an empty value here, and make reportEvent to don't set this value in this case.

Suggested change

m.reportEvent(merged, reporter, "MULTIPLE")

m.reportEvent(merged, reporter, "")

Made it empty.

* Make sql.query as “” for merge_results * Updated merge example in doc with queries from two separate tables.\ * Formatted query in test

…y/beats into generic-sql-event-merge-1

jsoriano

👍

jsoriano · 2022-07-21T08:41:29Z

x-pack/metricbeat/module/sql/_meta/docs.asciidoc

+    "address": "localhost"
+  },
+  "sql": {
+    "query": "",


Nit. I was suggesting to don't set this value if it is empty.

Suggested change

"query": "",

This does complicate the code for reportEvent, but fixed as asked 😃

jsoriano · 2022-07-21T09:36:28Z

x-pack/metricbeat/module/sql/query/query.go

@@ -187,20 +187,32 @@ func (m *MetricSet) Fetch(ctx context.Context, reporter mb.ReporterV2) error {
 func (m *MetricSet) reportEvent(ms mapstr.M, reporter mb.ReporterV2, qry string) {
 	if m.Config.RawData.Enabled {



Another way to do this would be:

moduleFields := mapstr.M{ "metrics": ms, // Individual metric "driver": m.Config.Driver, } if qry != "" { moduleFields["query"] = qry } reporter.Event(mb.Event{ ModuleFields: moduleFields, })

Yes. I hope we can stay with the current structure.

I mean, you can do it this way in case you feel it complicates less the code 🙂 #32394 (comment)

jsoriano

Apart of some nitpicking on Fetch and reportEvent, it LGTM, I will leave the final approve to the team.

jsoriano · 2022-07-21T14:11:54Z

x-pack/metricbeat/module/sql/query/query.go

+			} else {
+				// Report immediately for non-merged cases.
+				m.reportEvent(ms, reporter, q.Query)
+			}


Nit. This is getting more complicated now, maybe the code for variable and table format could be moved to separated functions.

ManojS-shetty

Tested with the few scenarios for MSSQL and able to see merged events after setting the flag merge_results: true
LGTM.

SQL events merge * Add flag merge_results to merge results for sql_queries. * Collect metrics when request_merge is enabled and report collectively at the end. * Flag error/warning when conditions not met. * Do not add sql.query when for merge_results used. * Updated documentation. * Fixed integration to add merged mode. Fixed earlier integration test issues.

lalit-satapathy added 5 commits July 17, 2022 08:49

SQL events merge

a201f83

* Add flag request_merge to request merge for sql_queries. * Collect metrics when request_merge is enabled and report collectively at the end. * Flag error/warning when conditions not met.

SQL events merge:

d9cc266

* Add flag request_merge to request merge for sql_queries. * Collect metrics when request_merge is enabled and report collectively at the end. * Flag error/warning when conditions not met.

Updated documentation.

ec73740

Fixed integration to add merged mode. Fixed earlier integration test issues.

changelog and remove extra comment.

66b810d

Remove sql.yml wrongly added.

bd816b0

lalit-satapathy added enhancement 8.4-candidate Team:Service-Integrations Label for the Service Integrations team labels Jul 19, 2022

lalit-satapathy requested a review from a team July 19, 2022 03:44

lalit-satapathy self-assigned this Jul 19, 2022

botelastic bot added needs_team Indicates that the issue/PR needs a Team:* label and removed needs_team Indicates that the issue/PR needs a Team:* label labels Jul 19, 2022

updated pull request.

211e617

lalit-satapathy requested review from ManojS-shetty and muthu-mps July 19, 2022 05:54

Merge branch 'main' into generic-sql-event-merge-1

9d08c54

lalit-satapathy marked this pull request as ready for review July 21, 2022 04:35

lalit-satapathy requested a review from a team as a code owner July 21, 2022 04:35

lalit-satapathy requested a review from jsoriano July 21, 2022 04:35

jsoriano reviewed Jul 21, 2022

View reviewed changes

lalit-satapathy added 2 commits July 21, 2022 13:31

* Renamed request_merge to merge_results

7725de3

* Make sql.query as “” for merge_results * Updated merge example in doc with queries from two separate tables.\ * Formatted query in test

Merge branch 'generic-sql-event-merge-1' of github.com:lalit-satapath…

4c7b310

…y/beats into generic-sql-event-merge-1

jsoriano reviewed Jul 21, 2022

View reviewed changes

* sql.query not populated for merged_results case.

dc5ca3a

jsoriano reviewed Jul 21, 2022

View reviewed changes

Merge branch 'main' into generic-sql-event-merge-1

212f651

jsoriano reviewed Jul 21, 2022

View reviewed changes

ManojS-shetty approved these changes Jul 22, 2022

View reviewed changes

lalit-satapathy merged commit ef523eb into elastic:main Jul 22, 2022

	MergeQueries bool `config:"request_merge"`
	MergeResults bool `config:"merge_results"`

	MergeQueries bool `config:"request_merge"`
	SingleEvent bool `config:"single_event"`

	m.reportEvent(merged, reporter, "MULTIPLE")
	m.reportEvent(merged, reporter, "")

		@@ -187,20 +187,32 @@ func (m *MetricSet) Fetch(ctx context.Context, reporter mb.ReporterV2) error {
		func (m *MetricSet) reportEvent(ms mapstr.M, reporter mb.ReporterV2, qry string) {
		if m.Config.RawData.Enabled {

Conversation

lalit-satapathy commented Jul 19, 2022

What does this PR do?

Why is it important?

Pending

Checklist

Author's Checklist

How to test this PR locally

Related issues

Uh oh!

mergify bot commented Jul 19, 2022

Uh oh!

elasticmachine commented Jul 19, 2022 • edited by jenkins-beats-ci bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

💚 Build Succeeded

Build stats

Test stats 🧪

💚 Flaky test report

🤖 GitHub comments

Uh oh!

mergify bot commented Jul 20, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jsoriano left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jsoriano Jul 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jsoriano left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ManojS-shetty left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

elasticmachine commented Jul 19, 2022 •

edited by jenkins-beats-ci bot

Loading

jsoriano Jul 21, 2022 •

edited

Loading