-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Performance Metricset on X-Pack MSSQL Metricbeat module #9826
Performance Metricset on X-Pack MSSQL Metricbeat module #9826
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Already looking good. Left some minor comments. Can you add a changelog entry? It can be combined with the entry for mssql.
f9b23ec
to
6c5a634
Compare
After a short chat with @ruflin we have decide to split this metricset in two. The reason was that the current metrics being fetch have mixed data from instances and databases. In short, we are going to leave all instance based performance metrics in this metricset and create a new metricset to retrieve database specific performance counters. At the same time, instead of doing a query to retrieve each performance value, we are using a Finally, the code will be more self contained and less abstracted so each metricset will have full independency of manipulation instead of being under the hood of the SQL framework that is built within |
err = errors.Wrap(err, "error doing ping to db") | ||
} | ||
|
||
return db, err |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This db instance is closed at the Metricset, I'm not fully sure if this is correct.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the connection per metricset? If yes, then this sounds correct.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right now it is per Metricset yes. I wasn't sure if that was correct either 😅 but it looked like the best way to isolate errors between metricset (that each has it's own connection).
objectName string | ||
instanceName string | ||
counterName string | ||
counterValue *int64 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All values returned by Performance Counters table are BIGINT
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a few minor comments.
Could you make Hound happy?
func (m *MetricSet) Fetch(reporter mb.ReporterV2) { | ||
var err error | ||
var rows *sql.Rows | ||
rows, err = m.db.Query("SELECT object_name, counter_name, instance_name, cntr_value FROM sys.dm_os_performance_counters WHERE counter_name = 'SQL Compilations/sec' OR counter_name = 'SQL Re-Compilations/sec' OR counter_name = 'User Connections' OR counter_name = 'Page splits/sec' OR (counter_name = 'Lock Waits/sec' AND instance_name = '_Total') OR counter_name = 'Page splits/sec' OR (object_name = 'SQLServer:Buffer Manager' AND counter_name = 'Page life expectancy') OR counter_name = 'Batch Requests/sec' OR (counter_name = 'Buffer cache hit ratio' AND object_name = 'SQLServer:Buffer Manager') OR (counter_name = 'Lock Waits/sec' and instance_name = '_Total')") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works for now. I remember for all these fields 1 column was empty (don't remember which one). Perhaps to simplify the query we could just check if this column is empty?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the query without WHERE
returns 2000 rows, 1000 are with the empty column. I guess that what you want is to simplify (automatize) the process of adding more fields. What we could do is to ask for everything (those 1000 rows), and take only the values we want from an array but we will still have to download 1000 rows from the db
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. I would probably download the 1000 rows but I don't know the size of it. Curious what would be less load on the DB.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, I have just checked and they are 273 rows and doing some ugly maths by checking the size of each cell, it will be a total of 109200 bytes to download.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
100KB sounds doable but let's keep it as is for now and "improve" it in case it ever becomes an issue.
func (m *MetricSet) Fetch(reporter mb.ReporterV2) { | ||
var err error | ||
var rows *sql.Rows | ||
rows, err = m.db.Query("SELECT object_name, counter_name, instance_name, cntr_value FROM sys.dm_os_performance_counters WHERE counter_name = 'SQL Compilations/sec' OR counter_name = 'SQL Re-Compilations/sec' OR counter_name = 'User Connections' OR counter_name = 'Page splits/sec' OR (counter_name = 'Lock Waits/sec' AND instance_name = '_Total') OR counter_name = 'Page splits/sec' OR (object_name = 'SQLServer:Buffer Manager' AND counter_name = 'Page life expectancy') OR counter_name = 'Batch Requests/sec' OR (counter_name = 'Buffer cache hit ratio' AND object_name = 'SQLServer:Buffer Manager') OR (counter_name = 'Lock Waits/sec' and instance_name = '_Total')") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. I would probably download the 1000 rows but I don't know the size of it. Curious what would be less load on the DB.
performance
metricset is a series of customized queries to extract very relevant information using thesys.dm_os_performance_counters
database (more information here).This concurrent structure was based on the fact that each fetch operation needs to establish a new connection to the database. Now that the connection is maintained active, a transport channel could be used to report events and maybe the code would be a bit simpler (but I need to be sure about a couple of things of how the framework works before doing it to not leave dangling channels by accident)
GA
data.json
exists and an automated way to generate it exists (go test -data
)