transaction_log Metricset for MSSQL Module (also db metricset removal)

[sayden]

Summary

• transaction_log Metricset in MSSQL module.
• Replaces / Removes "old" db Metricset.
• Removes references to "old" fetcher.go file with the abstractions that we'll use in the SQL helper.

transaction_log Metricset

Includes information about the log space usage and general log stats like backup size, time elapsed since last backup or log size since last backup.

Removal / Renaming of db Metricset

After careful anaysis, db naming had 3 problems:

• You can merely put everything in a db metricset because, all in all, 90% of metrics are part of a database.
• db means nothing and everything. A user will face the question "do I want to activate db metricset? Yes, of course, always! I want metrics from the database!" So the answer was always "Yes".
• 80% of the data that was included was about the database log and file space usage so I left only the metrics that were about the log (and left file space usage for a different metricset).

Removal of fetcher.go file

I was using an abstraction layer to perform, parse and emit metrics based on the queries that were provided. We decided to make it more explicit and less abstract to improve clarity and maintenance. The only metricset using the abstraction layer was db so now that it's removed, the file could be removed too.

It also forced to remove a forgotten reference in performance metricset which is included in this PR.

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)

[ruflin]

+1 on this change. Left a few minor comments.

[jsoriano]

Great change, everything looks much more clear now 🙂

[jsoriano]

Thanks for addressing all the comments, I have added some extra observations, let me know what you think. In any case I think this is good to go 👍

[jsoriano]

Not sure of the benefits of using named return values here, but ok.

[ruflin]

I personally prefer not to use named returns and only if it is really need like in defer functions. It makes from my perspective code more readable. @sayden could you update this?

[sayden]

Changed. I see the benefits and I also have my own preferences. Now it has more code and it's not super clear that it's returning a nil at the end (if you don't look up at the return values... but I thought that this is what we wanted to avoid).

Please, this have been like this for days if not weeks, next time, try to make this comments earlier.

[ruflin]

Thanks for adjusting it, appreciate it. Not sure i can follow your comment above as now if I look at the end of the method it's very clear, no error is returned.

Sorry that we missed that in previous reviews but I think initially we focused on the high level details like naming before going into the details.

Requested changes addressed.

[ruflin]

LGTM.

I think it needs an other make update.

[sayden]

I was running mage fmt update in Xpack folder instead of make update from OSS folder. Does it make sense that I need to do something in the OSS folder to make an X-Pack module work? Just curious

[ruflin]

Good question. As all the docs are in OSS the answer is probably yes for now but agree it's not optimal. We should clean this up more as soon as we migrated all to mage.

@andrewkroh This might also interest you.

[ruflin]

CI failures should not be related.