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

SQL Server input plugin : sqlserver_azurestats measurement field type is not float #6397

Closed
mygon0172 opened this issue Sep 16, 2019 · 6 comments · Fixed by #6869
Closed
Labels
area/sqlserver breaking change Improvement to Telegraf that requires changes to the plugin or agent; for minor/major releases feature request Requests for new plugin and for new features to existing plugins
Milestone

Comments

@mygon0172
Copy link

Feature Request

Proposal:

I would like the field type of sqlserver_azurestats measurement to be float.

Current behavior:

All field key types are String.

influx query : SHOW FIELD KEYS FROM sqlserver_azurestats

fieldKey fieldType
avg_cpu_percent string
avg_data_io_percent string
avg_log_write_percent string
avg_memory_usage_percent string
max_session_percent string
max_worker_percent string
xtp_storage_percent string

Desired behavior:

Requests to change to float type.

@danielnelson danielnelson added area/sqlserver feature request Requests for new plugin and for new features to existing plugins breaking change Improvement to Telegraf that requires changes to the plugin or agent; for minor/major releases labels Sep 16, 2019
@danielnelson
Copy link
Contributor

This seems like a good change, if we do this we would need to change the measurement name as well to prevent type conflicts.

cc @denzilribeiro @m82labs

@mygon0172
Copy link
Author

Thank you @danielnelson It is not a problem to change the measurement name.

@denzilribeiro
Copy link
Contributor

That is weird as the data type itself is Decimal - executing this in SQL you can check - and the query itself doesn't do any type conversion to string
sp_help 'sys.dm_db_resource_stats'
Given that not sure how it is converting to string when storing in influx
Code here :

const sqlAzureDBResourceStats string = `SET DEADLOCK_PRIORITY -10;

@danielnelson
Copy link
Contributor

I think I've gotten to the bottom of this, the go-mssqldb library scans these columns to a []uint8 when provided an interface{}, which Telegraf converts this to a string.

What this means that in addition to a query we need to create a list of the types that will be returned, ideally for each query but we could also ease into it one query at a time. We'll get this started by converting this one query, but we will need to pick a new measurement name.

We could name it sqlserver_azurestats_v2, or here is our chance to improve the name, maybe sqlserver_azure_db_resource_stats?

@denzilribeiro
Copy link
Contributor

Changing measurement name would break dashboards and this is used for Managed instance as well - I am personally not for a change in measurement name data as strings is incorrect at best. Wouldn't existing users be fine dropping that measurement?

@danielnelson
Copy link
Contributor

Unfortunately, type changes are too messy to transition if you have multiple Telegraf running, as you need to stop all Telegraf and then drop the measurement, then bring them all up again. We have also had issues where measurements are not fully dropped by InfluxDB influxdata/influxdb#9694 and it can result in a database which isn't compatible with both the new and old types.

We could continue to emit both measurements so that old dashboards still work, and give time to transition to the new measurement name. Maybe we have the fixed version be a separate query in the configuration file.

I suppose another option is renaming the fields but that doesn't think address you concern either, and is kind of ugly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/sqlserver breaking change Improvement to Telegraf that requires changes to the plugin or agent; for minor/major releases feature request Requests for new plugin and for new features to existing plugins
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants