MS SQL DB metricset #8250

sayden · 2018-09-05T21:17:32Z

I've started with this particular query https://docs.microsoft.com/en-us/sql/relational-databases/system-dynamic-management-views/sys-dm-db-file-space-usage-transact-sql?view=sql-server-2017 from the Database metricset.

I still have to build a local environment to test the entire module. I'll keep pushing progress and request approval once small metricsets are being finished

jsoriano

I have added some comments.

Also as it seems like a potentially big module, I suggest you to start by creating just a metricset, and then continue adding metricsets in follow up PRs, this will help getting this merged.

jsoriano · 2018-09-06T10:27:00Z

metricbeat/module/mssql/io/io.go

+
+func (m *MetricSet) loadFileSpaceUsage(db *sql.DB) (common.MapStr, error) {
+	// Returns the global status, also for versions previous 5.0.2
+	rows, err := db.Query("SELECT * FROM sys.dm_db_file_space_usage;")


You can probably reuse this query between all metricsets, move it to a common place at the module level.

This could probably be a method of a common MetricSet type.

Moved to a different function to perform queries concurrently, check fb7cc98#diff-333a1912b4f61b216d6a3fd05fd647e7R123 on latest commit

jsoriano · 2018-09-06T10:29:40Z

metricbeat/module/mssql/io/data.go

+			"drive_name":  c.Str("DriveName"),
+			"pdw_node_id": c.Int("pdw_node_id"),
+		},
+		//Returns a row for each pending I/O request in SQL Server.


If there is a different row for each request, then it'd be better to move them to their own metricset, and generate an event for each one of them.

Here you can keep a summary in any case, with the count of pending requests, total pending ticks...

I haven't done this yet on the current commit. I need to investigate a bit more how it works :) but I think I understand the overall idea

jsoriano · 2018-09-06T10:35:59Z

metricbeat/module/mssql/io/data.go

+		// fn_virtualfilestats function.
+		"virtual_file_stats": s.Object{
+			"database_name":            c.Str("database_name"),
+			"database_id":              c.Int("database_id"),


Fields that are going to exist in different metricsets should have the same name, this helps on correlation. These database fields are good candidates for that, after applying the schema you can move them to the module level.

Look at some examples that use the ModuleFields attribute.

after applying the schema you can move them to the module level don't really understand what do you mean here. Can I apply and then modify the schema to move / delete most ocurrences of the same field to a parent s.Object?

Oh, yes, I mean that after applying the schema you can still modify the resulting object, for example deleting some fields after copying them to ModuleFields.

jsoriano · 2018-09-06T10:37:15Z

metricbeat/module/mssql/io/data.go

+		// fn_virtualfilestats function.
+		"virtual_file_stats": s.Object{
+			"database_name":            c.Str("database_name"),
+			"database_id":              c.Int("database_id"),


You could structure these fields in an object:

"database": s.Object{ "name": c.Str("database_name"), "id": c.Int("database_id"), }

Because now there is only a single group of metrics in this package, I haven't done it to simplify the tree.

Well, even with an only metricset I think this would be something good to do in any case.

Done in e151780d30f4db3cca88511767a134f295976607

metricbeat/module/mssql/io/data.go

jsoriano · 2018-09-06T10:43:31Z

metricbeat/module/mssql/io/io.go

+	mb.BaseMetricSet
+	counter int
+	db      *sql.DB
+}


The MetricSet object is probably going to be reusable between metricsets of this module, take a look for example to the mongodb module where there is a common metricset defined at the module level.

Done! fb7cc98#diff-eddaad12b395e1f0a6179ce4f2ab9dbfR17

jsoriano · 2018-09-06T10:44:47Z

metricbeat/module/mssql/db/_meta/data.json

+        }
+    },
+    "type":"metricsets"
+}


Take a look to the TestData functions in the integration tests of other modules to see how this file is generated.

houndci-bot · 2018-09-07T08:06:49Z

metricbeat/module/mssql/mssql.go

+	return &MetricSet{Db: db, BaseMetricSet: base}, nil
+}
+
+func NewModule(base mb.BaseModule) (mb.Module, error) {


exported function NewModule should have comment or be unexported

houndci-bot · 2018-09-07T08:06:49Z

metricbeat/module/mssql/mssql.go

+	Port     int    `config:"port" validate:"nonzero,required"`
+}
+
+func NewMetricSet(base mb.BaseMetricSet) (*MetricSet, error) {


exported function NewMetricSet should have comment or be unexported

houndci-bot · 2018-09-07T08:06:50Z

metricbeat/module/mssql/mssql.go

+	Db *sql.DB
+}
+
+type Config struct {


exported type Config should have comment or be unexported

houndci-bot · 2018-09-07T08:06:50Z

metricbeat/module/mssql/mssql.go

+	}
+}
+
+type MetricSet struct {


exported type MetricSet should have comment or be unexported

houndci-bot · 2018-09-07T08:06:50Z

metricbeat/module/mssql/mssql.go

+import (
+	"database/sql"
+	"fmt"
+	_ "github.com/denisenkom/go-mssqldb"


a blank import should be only in a main or test package, or have a comment justifying it

houndci-bot · 2018-09-07T08:06:50Z

metricbeat/module/mssql/db/db.go

+import (
+	"database/sql"
+	"fmt"
+	_ "github.com/denisenkom/go-mssqldb"


a blank import should be only in a main or test package, or have a comment justifying it

jsoriano

Added some more comments. I think it'd be better if you start by creating just a simple metricset and then continue expanding this metricset and adding new ones in follow up PRs.

jsoriano · 2018-09-07T09:11:38Z

metricbeat/module/mssql/db/db.go

+		if c.error == nil {
+			c.error = err
+		} else {
+			c.error = errors.Wrap(c.error, err.Error())


I think you are looking for multierror here, not errors.Wrap 🙂

jsoriano · 2018-09-07T09:13:51Z

metricbeat/module/mssql/db/db.go

+// Fetch methods implements the data gathering and data conversion to the right
+// format. It publishes the event which is then forwarded to the output. In case
+// of an error set the Error field of mb.Event or simply call report.Error().
+func (m *MetricSet) Fetch() ([]common.MapStr, error) {


There are multiple interfaces a metricset can implement depending on the signature of its Fetch method, this one returning the event is the "old" one, we currently prefer the ReportingMetricSetV2 interface, where the Fetch method receives a ReporterV2 object that can be used to send any number of events and errors.

Take a look to its usage in other modules, it'd be nice to use it on new modules like this one.

jsoriano · 2018-09-07T09:20:57Z

metricbeat/module/mssql/db/db.go

+
+	defer func() {
+		if closeErr := rows.Close(); closeErr != nil {
+			//TODO Log error? Ignore it?


I think that just defer rows.Close().

jsoriano · 2018-09-07T09:21:24Z

metricbeat/module/mssql/db/db.go

+	c.maprs = append(c.maprs, maprSlice...)
+}
+
+func doQuery(db *sql.DB, query string) ([]common.MapStr, error) {


If you want this method to be reused between metricsets put it at the module level.

jsoriano · 2018-09-07T09:22:56Z

metricbeat/module/mssql/io/io.go

+	return mapR, nil
+}
+
+func rowsToMapR(rows *sql.Rows) (common.MapStr, error) {


We use to call this method eventMapping in other modules 🙂

Done b905b46a3a44451bcc1aebe18455431b1667939c

jsoriano · 2018-09-07T09:25:43Z

metricbeat/module/mssql/io/io.go

+// Fetch methods implements the data gathering and data conversion to the right
+// format. It publishes the event which is then forwarded to the output. In case
+// of an error set the Error field of mb.Event or simply call report.Error().
+func (m *MetricSet) Fetch(report mb.ReporterV2) (common.MapStr, error) {


Oh, you are already using ReporterV2 here, but fetch here shouldn't return anything, use report.Event() to send an event and report.Error() to send an error.

Done here 9c900938162dc77e08b70b1bd8b080b26c7bcd7e, althought I'm a bit confusing now because I'm not fully sure about how to create integration tests

Integration test done here 069cfb5cda5baf8574bf0941241014089cd74db8

jsoriano · 2018-09-07T09:31:52Z

metricbeat/module/mssql/db/db.go

+		return nil, err
+	}
+
+	//TODO Db must be gracefully closed


Not sure if needed here, you probably want to connect and disconnect on Fetch
But if you want to start something in New you can implement also the Closer interface in the MetricSet to do the cleanup when it is stopped.

I was actually thinking how was the lifetime of a metricset and a module

I have seen that you have added a Close method, but as I said you probably want to connect and disconnect on Fetch. The main problem of leaving the connection open during all the life of the metricset is that you need to handle reconnections if the connection is closed by the server or any other external reason. This can add unnecessary complexity.

jsoriano · 2018-09-07T09:32:28Z

metricbeat/module/mssql/db/db.go

+		"dm_db_persisted_sku_features", //TODO Returns nothing with empty db
+		"dm_db_session_space_usage",
+		"dm_db_task_space_usage",
+		"dm_db_uncontained_entities", //TODO Returns nothing using the driver. Works with sqlcmd


Are all these queries needed for a simple metricset? :)

houndci-bot · 2018-09-07T11:05:44Z

metricbeat/module/mssql/fetcher.go

+	"sync"
+)
+
+func NewFetcher(db *sql.DB, qs []string, schema *s.Schema) *fetcher {


exported function NewFetcher should have comment or be unexported
exported func NewFetcher returns unexported type *mssql.fetcher, which can be annoying to use

houndci-bot · 2018-09-07T11:05:45Z

metricbeat/module/mssql/db/db.go

+package db
+
+import (
+	_ "github.com/denisenkom/go-mssqldb"


a blank import should be only in a main or test package, or have a comment justifying it

sayden · 2018-09-07T11:09:24Z

Ok. metricset got really simplified now and the io.Close implementation closes the db connection.

Most of the logic is now in package mssql in the fetcher.go file that will help every metricset in doing queries concurrently to the database.

houndci-bot · 2018-09-07T22:10:28Z

metricbeat/module/mssql/mssql.go

+	return sql.Open("sqlserver", u.String())
+}
+
+func NewModule(base mb.BaseModule) (mb.Module, error) {


exported function NewModule should have comment or be unexported

houndci-bot · 2018-09-07T22:10:28Z

metricbeat/module/mssql/mssql.go

+	return &MetricSet{BaseMetricSet: base}, nil
+}
+
+func NewDB(config *Config) (*sql.DB, error) {


exported function NewDB should have comment or be unexported

houndci-bot · 2018-09-07T22:10:28Z

metricbeat/module/mssql/mssql.go

+	mb.BaseMetricSet
+}
+
+type Config struct {


exported type Config should have comment or be unexported

Experimental `db` metricset Move methods to call SQL concurrently to the mssql package so it's available to all metricsets Implement io.Close to do cleanup on the db metricset Rename function rowsToMapR to eventsMapping Promote database_id to the parent level within the schema DB connection is created and closed on each Fetch call. Minor improvements on pointer receiver namings on fetcher Updated data.json when launching go tests with -data Added comment to NewDB function. Make private newModule function as it is only used within mssql package but not from any metricset db_integration_test.go rewritten to work with ReporterV2 Removed unused variable and fix typo Remove metricset io from this PR and added license headers Moved files to X-Pack folder Delete references to mssql Squash into a single commit after moved files to x-pack folder

houndci-bot · 2018-09-12T16:42:04Z

x-pack/metricbeat/module/mssql/mssql.go

+	Port     int    `config:"port" validate:"nonzero,required"`
+}
+
+func NewMetricSet(base mb.BaseMetricSet) (*MetricSet, error) {


exported function NewMetricSet should have comment or be unexported

houndci-bot · 2018-09-12T16:42:04Z

x-pack/metricbeat/module/mssql/mssql.go

+	mb.BaseMetricSet
+}
+
+type Config struct {


exported type Config should have comment or be unexported

houndci-bot · 2018-09-12T16:42:04Z

x-pack/metricbeat/module/mssql/mssql.go

+	}
+}
+
+type MetricSet struct {


exported type MetricSet should have comment or be unexported

houndci-bot · 2018-09-12T16:42:05Z

x-pack/metricbeat/module/mssql/mssql.go

+import (
+	"database/sql"
+	"fmt"
+	_ "github.com/denisenkom/go-mssqldb"


a blank import should be only in a main or test package, or have a comment justifying it

houndci-bot · 2018-09-12T16:42:05Z

x-pack/metricbeat/module/mssql/fetcher.go

+	"sync"
+)
+
+func NewFetcher(config *Config, qs []string, schema *s.Schema) (*fetcher, error) {


exported function NewFetcher should have comment or be unexported
exported func NewFetcher returns unexported type *mssql.fetcher, which can be annoying to use

houndci-bot · 2018-09-12T16:42:05Z

x-pack/metricbeat/module/mssql/db/db.go

+package db
+
+import (
+	_ "github.com/denisenkom/go-mssqldb"


a blank import should be only in a main or test package, or have a comment justifying it

houndci-bot · 2018-10-02T11:08:04Z

x-pack/metricbeat/module/mssql/server/server.go

+package server
+
+import (
+	_ "github.com/denisenkom/go-mssqldb"


a blank import should be only in a main or test package, or have a comment justifying it

houndci-bot · 2018-10-05T09:44:01Z

metricbeat/module/mssql/server/server.go

+package server
+
+import (
+	_ "github.com/denisenkom/go-mssqldb"


a blank import should be only in a main or test package, or have a comment justifying it

houndci-bot · 2018-10-05T09:44:02Z

metricbeat/module/mssql/fetcher.go

+	"github.com/pkg/errors"
+)
+
+func NewFetcher(config *Config, qs []string, schema *s.Schema) (*fetcher, error) {


exported function NewFetcher should have comment or be unexported
exported func NewFetcher returns unexported type *mssql.fetcher, which can be annoying to use

andrewkroh · 2018-10-31T01:21:42Z

@sayden I started working on adding magefile.go for building x-pack/metricbeat. You can view that work at https://github.com/andrewkroh/beats/commits/sayden-mssql-metricset. I had to add some dependencies that were missing to vendor/.

With my changes you can do a few things:

mage update - Generates:
- fields.yml
- fields.go for each x-pack module
- dashboard and index patterns to build/kibana
- metricbeat.yml
- metricbeat.reference.yml
- modules.d/ dir
- include/list.go
SNAPSHOT=true mage package - build snapshot packages

I'm working on the integration testing piece, but I can run them with a bit of manual work.

mkdir build
GOOS=linux GOARCH=amd64 mage -f -compile build/mage-linux-amd64
docker-compose run -e TEST_COVERAGE=true -e RACE_DETECTOR=true -e MAGEFILE_VERBOSE=1 beat /go/src/github.com/elastic/beats/x-pack/metricbeat/build/mage-linux-amd64 goTestIntegration

ruflin · 2018-10-31T06:41:42Z

@andrewkroh @sayden Perhaps you can collaborate on #8829 ?

andrewkroh · 2018-10-31T23:37:36Z

@sayden @ruflin I saw #8829 right after I pushed my commit when I started checking emails.

My end goal is to replace most of the shared build logic that exists in Makefiles and python scripts with Go (I don't care so much about porting to Go things that are not shared/reused across Beats like create-metricset).

Rather than doing it all at once my approach has been to avoid changing any Makefiles (or other Beats) to the extent that this is possible, re-create any scripts I need with Go, and execute the build with Mage (we started with x-pack/filebeat). Once we prove this is working by being able to build/test/package from x-pack/Metricbeat with mage then we can start expanding to the next Beat that is adding content with x-pack/ which is Auditbeat.

Then I think we'll be in a good position to start updating the other Beat's magefiles to remove the dependency on Make. In the end we should be able to fully build and test from the magefile and even on Windows.

To that extend I think some of the most important pieces to add to be complete for x-pack/Metricbeat are:

collect the module docs and generate an asciidoc index that includes each module's docs (e.g. mage docs). I skipped this for Filebeat, but it needs the same thing.
add a mage target for code formating and adding license headers
standardize the testing targets: unitTest, goUnitTest, pythonUnitTest, integTest, goIntegTest, pythonUnitTest. Add goTestUnit and goTestIntegration to magefile #7766 (comment)
Be able to execute the integ tests within Docker when required (I started on this)

Perhaps we can join a call tomorrow or Friday and discuss how we can collaborate to get your MSSQL module building, testing, and packaged as soon as possible (I know you've been waiting for a while).

sayden · 2018-11-01T21:29:54Z

@andrewkroh I think that the approach you describe is the best for all, specially to remove some of the black magic and silent errors we currently have in Python / Makefiles.

I have already done the changes needed for formatting and license headers in Makefile / Python so I can start helping you in this with Mage. I'm currently very close to finish make update command which could be helpful to start building more x-pack modules ASAP (at least as a development environment).

I was looking at the fields mage command right now so I think it's a good moment to start playing with Mage too, as you can see in this #8829 I have reached a good understanding of what some our current scripts do.

sayden · 2018-12-05T10:07:38Z

Continuing here #9202

sayden added in progress Pull request is currently in progress. module Metricbeat Metricbeat needs tests needs_docs labels Sep 5, 2018

sayden requested a review from jsoriano September 5, 2018 21:17

sayden changed the title ~~Atomic commit of the current work in progress MS SQL metricset~~ MS SQL metricset Sep 6, 2018

jsoriano reviewed Sep 6, 2018

View reviewed changes

houndci-bot reviewed Sep 7, 2018

View reviewed changes

jsoriano reviewed Sep 7, 2018

View reviewed changes

houndci-bot reviewed Sep 7, 2018

View reviewed changes

sayden changed the title ~~MS SQL metricset~~ MS SQL DB metricset Sep 10, 2018

sayden closed this Sep 11, 2018

sayden deleted the mssql-metricset branch September 11, 2018 20:51

sayden restored the mssql-metricset branch September 12, 2018 07:37

sayden reopened this Sep 12, 2018

sayden force-pushed the mssql-metricset branch from d46192b to aef8daa Compare September 12, 2018 10:35

sayden force-pushed the mssql-metricset branch from aef8daa to 6ac24df Compare September 12, 2018 14:39

Change headers of files to show the proper license

996b7db

houndci-bot reviewed Sep 12, 2018

View reviewed changes

sayden added 2 commits October 2, 2018 13:05

MS SQL Server metricset initial commit for PR review

d79badb

Move files to x-pack folder

aaee852

houndci-bot reviewed Oct 2, 2018

View reviewed changes

Atomic commit of mssql progress

69c64b0

houndci-bot reviewed Oct 5, 2018

View reviewed changes

sayden mentioned this pull request Oct 23, 2018

MS SQL Metricbeat module #8698

Closed

33 tasks

ruflin added the Team:Integrations Label for the Integrations team label Nov 21, 2018

andrewkroh mentioned this pull request Nov 21, 2018

Microsoft SQL Server Module for Metricbeat #9202

Closed

8 tasks

ruflin added Team:Integrations Label for the Integrations team and removed Team:Integrations Label for the Integrations team labels Nov 27, 2018

ruflin assigned sayden Dec 3, 2018

sayden closed this Dec 5, 2018

MS SQL DB metricset #8250

MS SQL DB metricset #8250

Conversation

sayden commented Sep 5, 2018

jsoriano left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsoriano left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sayden commented Sep 7, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewkroh commented Oct 31, 2018 • edited Loading

ruflin commented Oct 31, 2018

andrewkroh commented Oct 31, 2018 • edited Loading

sayden commented Nov 1, 2018

sayden commented Dec 5, 2018

andrewkroh commented Oct 31, 2018 •

edited

Loading

andrewkroh commented Oct 31, 2018 •

edited

Loading