Skip to content
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

sqlserver plugin should have database name as tag and not in metric name #1816

Closed
StianOvrevage opened this issue Sep 27, 2016 · 10 comments
Closed
Labels
breaking change Improvement to Telegraf that requires changes to the plugin or agent; for minor/major releases feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin
Milestone

Comments

@StianOvrevage
Copy link
Contributor

Current behavior:

When using sqlserver plugin all per-database series are exported as follows:

State_OFFLINE_databaseA{host="XX",servername="XX:YY",type="Database properties"} 0
State_OFFLINE_databaseB{host="XX",servername="XX:YY",type="Database properties"} 0
State_OFFLINE_databaseB{host="XX",servername="XX:YY",type="Database properties"} 0

Desired behavior:

The database name should be exported as a tag instead:

State_OFFLINE{host="XX",servername="XX:YY", db="databaseA", type="Database properties"} 0
State_OFFLINE{host="XX",servername="XX:YY", db="databaseB", type="Database properties"} 0
State_OFFLINE{host="XX",servername="XX:YY", db="databaseB", type="Database properties"} 0

Use case:

That would allow for more dynamic queries and avoid need to hard-code metric queries including database-names.

I'm using telegraf with Prometheus at the moment and Prometheus has no way of querying State_OFFLINE_* for example. I'm guessing InfluxDB is also not built to have instance/database names etc as part of the metric name but as tags.

@sparrc sparrc added breaking change Improvement to Telegraf that requires changes to the plugin or agent; for minor/major releases feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin labels Sep 27, 2016
@sparrc
Copy link
Contributor

sparrc commented Sep 27, 2016

I agree that this would be better, but unfortunately it would be a breaking change.

@StianOvrevage
Copy link
Contributor Author

Ok. I tried to figure out how to change the plugin but since it's one huge (T?)SQL query I could not wrap my head around how it worked. Could you give me a hint on how to change the source code locally to shift database name to tags?

Also: Could it be an option to add the same metrics with tags in addition to in metric name, to conserve backwards compatibility?

@nhaugo nhaugo added this to the Future Milestone milestone Sep 28, 2016
@sparrc sparrc added the help wanted Request for community participation, code, contribution label Sep 28, 2016
@zensqlmonitor
Copy link
Contributor

zensqlmonitor commented Oct 21, 2016

@StianOvrevage @sparrc

Please do not change this.
With InfluxDB and within Grafana, database name is a field and can be easily queried like this:
SELECT "databasename" FROM "State OFFLINE" WHERE "servername" =~ /$Instance$/ AND "type" = 'Database properties' AND $timeFilter

Result is as follows:
measurement: 'State RESTORING'
servername: 'xxx'
type: 'Database properties'
master: 0
model: 0
tempdb: 0
total: 0

The result is formatted as a pivot to get sum info and individual databases value within the same row.
That's wanted and works properly with InfluxDB.
The result you want is a different need.

@zensqlmonitor
Copy link
Contributor

I will work on the mssqlserver_extensible plugin soon.

@StianOvrevage
Copy link
Contributor Author

I understand that it's easy with Influx but it's impossible with Prometheus. I'm also just proposing an extension of the plugin and not necessarily a breaking change. I've also tried changing the original queries in the mssqlserver plugin but it's just too complicated for me to understand.

I really appreciate you taking some time to look at it!

@sparrc
Copy link
Contributor

sparrc commented Oct 24, 2016

@zensqlmonitor I still think that the database name should be a tag, the more idiomatic way of doing this measurement would be to call the field value, then you could modify your original query as such:

SELECT value,databasename FROM "State OFFLINE" WHERE "servername" =~ /$Instance$/ AND "type" = 'Database properties' AND $timeFilter

this would also support the use-case of @StianOvrevage, who could filter specific databases with something like this:

SELECT value FROM "State OFFLINE" WHERE "databasename" = 'dbA' AND "servername" =~ /$Instance$/ AND "type" = 'Database properties' AND $timeFilter

@Garagoth
Copy link

This plugin is putting all simple metrics as measurement name, resulting in hundreds of names in "select from ...." dropdown. Normally plugins export metrics so they are all grouped under one plugin name (so I have hyper_v, cpu, mem, ...), then you have all the fields, tags... Even multiple fields for one metric, not just 'value', all tagged for ease of filtering.

Also, this plugin lists several groups of measurements, those are not tagged as well so it is impossible to see for example all CPU or Memory clerk metrics and nothing else (except regexp match on metric name....). Grafana for example forces me to first select measurement and then I am allowed to add WHERE clauses - in this case WHERE clauses will have single values possible (except servername)

For example, there are measurements:
Average Wait Time Base | OIB | Locks
You are adding into name several attributes separating then using pipe '|' - IMHO more sensible would be to put them as tags.

As for your example in selecting all database names from specified hosts:
Influx can return values of tags as well:
SELECT "databasename" FROM "State OFFLINE" WHERE "servername" =~ /$Instance$/ AND "type" = 'Database properties' AND $timeFilter
would be something like:
SHOW TAG VALUES WITH KEY = "databasename" WHERE servername =~ /$Instance$/ and type = 'Database properties'
Not to mention that selecting State OFFLINE will always have type of "Database properties"...
And then you have all databases as fields, so to show all you have to add field for each name manually and specifically.
Granted, from this query you will not get measurements, but to have that you can use select with grouping and end result will be similar... unless you have some very specific needs I do not understand.

Regards,
Marcin.

@sparrc
Copy link
Contributor

sparrc commented Jan 21, 2017

I agree that it should be a tag, if anyone can submit a PR it would be greatly appreciated

@danielnelson danielnelson removed this from the Future Milestone milestone Jun 14, 2017
@danielnelson
Copy link
Contributor

@StianOvrevage @Garagoth In 1.6 we are introducing a new output format for this plugin enabled by setting query_version = 2. I would be great if you could test it out with the nightly builds and provide any feedback.

Relevant pull request: #3618

@danielnelson danielnelson removed the help wanted Request for community participation, code, contribution label Mar 19, 2018
@danielnelson
Copy link
Contributor

Fixed in #3618

@danielnelson danielnelson added this to the 1.6.0 milestone Jul 10, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change Improvement to Telegraf that requires changes to the plugin or agent; for minor/major releases feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin
Projects
None yet
Development

No branches or pull requests

6 participants