MS SQL db Metricset (main PR)

[sayden]

Main PR suffix refers to the fact that this PR contains the first metricset plus the sql engine to do the queries concurrently. It's expected that follow PR with new metricset just contains the queries to fetch the telemetry information.

Continuation of #9202 which grouped 4 different metricsets.

This is the single db metricset to facilitate reviewing.

db metricset is a subset of some of the most relevant information you can find here.

The way it has to work is: the metricset has the SQL queries it needs to get the information from the DB. Queries are sent to a central engine created for each metricset in x-pack/metricbeat/module/mssql which run them, returning the results so that they can be reported. io.Closer is implemented on the engine to close the db connection and in the metricset.

GA

• Supported versions are documented
• Supported operating systems are documented (if applicable)
• Integration tests
• System tests
• Automated checks that all fields are documented
• Documentation
• Example data.json exists and an automated way to generate it exists (go test -data)
• Fields follow naming conventions: https://www.elastic.co/guide/en/beats/devguide/master/event-conventions.html
• Dashboards exists (if applicable)
• Kibana Home Tutorial (if applicable)

[elasticmachine]

Pinging @elastic/infrastructure

[ruflin]

It's up to you to make sure the connections are properly closed. Best check the mysql metricset on how it's done there.

[ruflin]

Only skimmed through the code and left some high level comments. Overall looks good 👍

[sayden]

NOTE: sa is the common name for system administrator in MS SQL

[jsoriano]

This is looking good 👍 Only some small comments.

[jsoriano]

Should this be the description of the db group?

[sayden]

No, actually not but just after your comment I thought for a second that you were right 😄 Many along different metricsets returns id's of databases and this group is specifically tailored to share this names between metricsets. This was actually some suggestion from you on one of the old PR.

What you make me doubt now is if the database group field should be place at the module level. But I actually don't know very well how this actually works. I mean, how do I apply the common eventMapping function of the data.go files to generate a field / group that belongs to the module? I thought that all event mapping produce there is automatically a subchild of the module group.

[jsoriano]

My comment here was only about their descriptions, I don't understand why db.database description is "Database metricset" and the one for db is "db". I'd say that db description should say something about the metricset metrics, and db.database something about database info, or just be empty.

Regarding organization, if database is going to be shared between metricsets as I think I suggested in some of the other PRs then yes, this should be placed at the module level, so the id is in mssql.database.id.

Notice that after applying an schema you can still modify the events before publishing them.

[ruflin]

Agree with @jsoriano that as soon as we see more metricsets, we potentially should redicuss how we organise the fields and which ones are on the module level.

[jsoriano]

@sayden I think these descriptions should be fixed on this PR

[sayden]

Waiting this #9509 to merge before rebasing and try again

[sayden]

jenkins, test this

[sayden]

Rebased over #9509

[sayden]

jenkins, test this

[ruflin]

I'm missing 2 things:

• An integration tests that can generated the data.json + the generated data.json as we use this in the docs
• A (system)test that checks that all fields are documented which we used.

[ruflin]

Should we add some more details here on what metrics are fetched?

[sayden]

I can but it's going to be redundant with the in fields.yml, right? I want to be sure that you mean that information

[ruflin]

Yes. I should rephrase this to not necessarly about the metrics but that it describes it fetches metrics about DB and also potentially links to MSSQL docs where these metrics are. We can do this in a follow up.

[ruflin]

Agree with @jsoriano that as soon as we see more metricsets, we potentially should redicuss how we organise the fields and which ones are on the module level.

[ruflin]

Could we use a name return value here instead so we don't have to log it and still get the error reported as return value?

[sayden]

Oh! Good catch! Because I was actually ofuscating err from any of the other calls in case of an error here. So, as far as I can see it could be done like this (but it's ugly as hell):

func (f *Fetcher) getEventsWithQuery(db *sql.DB, query string, reporter mb.ReporterV2) (err error) {
	var rows *sql.Rows
	rows, err = db.Query(query)
	if err != nil {
		return errors.Wrapf(err, "error performing db query='%v'", query)
	}
	defer func() {
		if err2 := rows.Close(); err != nil {
			if err != nil {
				err = errors.Wrap(err, err2.Error())
			} else {
				err = err2
			}
		}
	}()

[jsoriano]

Sorry, but as you said this is actually a bit ugly 😬 Also according to documentation rows are closed automatically by Next, so this should only be needed if the Scan/Next loop is unexpectedly interrupted (having rows creation and the Scan/Next loop in the same place would be another reason in favour of this proposal).

Also for better or worse it is quite usual to ignore the error on calls to Close(), even in the examples in the documentation for the sql module... I think we'd be fine with reducing this to defer Close(), or at much log this error at the debug level, because an error here is probably very very rare.

I'd change it here, but we will have to revisit this code in any case, so as you prefer... but if you keep this here, please at least use a more meaningful name for the inner error, like closeErr.

[sayden]

I think we should not block this PR with more stylish and subjective things.

Very very rare errors that are hidden on purpose leads to a very very difficult to debug, discover and fix bugs so also very very difficult to maintain. That's why I prefer explicit approaches (say ugly if you want) Than just to obfuscate the error.

So please, if you don't mind, let's move this PR forward because it has been hanging for months already, it has 81 comments and there's still has a lot of work to do with the upcoming metricsets that will need a lot of review in many aspects.

[ruflin]

I think the problem is that code that gets merge gets rarely changed again. My main issue with the above is that when I read the code, I don't really understand what it's doing above.

To keep this moving the following suggestion: Please add it to the module meta issue as an entry that has to be looked at again so we don't forget about the cleanup.

[sayden]

I'm happy with any proposal that doesn't involve a function with 4 or 5 responsibilities 🥇 In any case, I already have some notes for the next PR that I'm planning to write in the header of the upcoming metricset PR once it's open. I already have to change few stylish things also in this particular file so we have time to discuss a way that is easy for everybody to understand / look 😄

[ruflin]

LGTM. Let's move forward with this. I think it will still need some follow up work like for the fields description or docs but we are good to move forward here.

Lets wait on CI to go green(-ish).

@sayden One other cleanup thing we must do after merging this is remove the "example" module as we will not need it anymore.

[sayden]

The follow up actions of this PR are in this meta-issue about the entire module #8698 including the docs reviewing, the sql row close action error handling

[sayden]

jenkins, test this please

[sayden]

Trying to figure out what's the Travis error of cat: build/TEST-go-integration.out: No such file or directory

[sayden]

jenkins, test this please

[sayden]

jenkins, test this please

[ruflin]

@sayden Failures on CI are not related.