Updates to SQL Server Plugin, better Azure Managed Instance support + more #4642
Conversation
|
@danielnelson I changed a dependency, not sure how to get that added into the deps. |
|
I think we need to modify the Gopkg.toml by hand: After this run |
danielnelson
added
feat
|
I think we need |
|
Yep. I will get the new rep files committed shortly. This is more of an
enhancement than something addressing current issues. After this is merged
in I plan on working on a few of the open issues.
…On Thu, Sep 6, 2018 at 16:58 Daniel Nelson ***@***.***> wrote:
I think we need dep ensure still. Do you have a list of the issues this
addresses?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#4642 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AIS1dB4Jf3yDfSC1vLAA1Ec_HncEONlsks5uYYx_gaJpZM4WdXUk>
.
|
|
Maybe it matches some feature requests. Can you take a look at, being careful not to link yet, these issues: 4569, 4597, 4523, 4398, 4382 |
|
@danielnelson just added a fix for 4597, 4382 was already closed out via #4571 |
|
Looks like that pr was for 4398, closed now. |
|
@danielnelson I am assuming the build issue is not related to my changes? |
| ) | ||
| ) OR ( | ||
| ) OR ( |
| uptime INT | ||
| ) | ||
|
|
||
| IF OBJECT_ID('master.sys.dm_os_sys_info') IS NOT NULL | ||
| IF OBJECT_ID('master.sys.dm_os_sys_info') IS NOT NULL |
| AND waiting_tasks_count > 0 | ||
| ORDER BY | ||
| waiting_tasks_count DESC | ||
| AND wait_time_ms > 100 |
|
I’ll fix the spacing issues. The last item is intentional yes. It was
unintentional to leave the ORDER BY in there when I first developed that
query.
…On Mon, Sep 10, 2018 at 15:11 Greg ***@***.***> wrote:
***@***.**** approved this pull request.
------------------------------
In plugins/inputs/sqlserver/sqlserver.go
<#4642 (comment)>:
> )
- ) OR (
+ ) OR (
un-needed extra spacing.
------------------------------
In plugins/inputs/sqlserver/sqlserver.go
<#4642 (comment)>:
> uptime INT
)
-IF OBJECT_ID('master.sys.dm_os_sys_info') IS NOT NULL
+IF OBJECT_ID('master.sys.dm_os_sys_info') IS NOT NULL
un-needed extra spacing
------------------------------
In plugins/inputs/sqlserver/sqlserver.go
<#4642 (comment)>:
> AND waiting_tasks_count > 0
-ORDER BY
-waiting_tasks_count DESC
+AND wait_time_ms > 100
Is this intended?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#4642 (review)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AIS1dEZXheFFm1tbHB87lVcyIY-7v4_qks5uZrlFgaJpZM4WdXUk>
.
|
|
@m82labs Are there any README changes we should make with this change? |
|
I can add some verbiage about Azure Managed Instance support. That’s
probably one of the biggest changes.
…On Mon, Sep 10, 2018 at 18:38 Daniel Nelson ***@***.***> wrote:
@m82labs <https://github.com/m82labs> Are there any README changes we
should make with this change?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#4642 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AIS1dLCVXxT53eVfPzWsffE7zuKOOZXrks5uZunYgaJpZM4WdXUk>
.
|
|
Can you check if this resolves 4222? |
|
@danielnelson I think it does. I don't have anything to test it against, but I know when this was tested against Azure Managed Instances it would not work until the SQL package was changed. |
|
@m82labs One last question, all the changes are backwards compatible? New fields and tags are okay but are there any renames or type changes? |
|
I have not run into a case of this. I am currently running a build
including these changes in a dev environment. One change that could affect
people is that there are cases where more instances have been included for
a given counter, not just the total.
…On Tue, Sep 11, 2018 at 15:57 Daniel Nelson ***@***.***> wrote:
@m82labs <https://github.com/m82labs> One last question, all the changes
are backwards compatible? New fields and tags are okay but are there any
renames or type changes?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#4642 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AIS1dD-UmbexEcTBw6u9dnvYH5jHR1Nyks5uaBWwgaJpZM4WdXUk>
.
|
|
That should be okay since the new data would be tagged. |
|
It is incrementally a little more data collected but then also provides much deeper insights
Sent from mobile device
On Sep 11, 2018, at 8:41 PM, Mark Wilkinson - m82labs <notifications@github.com<mailto:notifications@github.com>> wrote:
I have not run into a case of this. I am currently running a build
including these changes in a dev environment. One change that could affect
people is that there are cases where more instances have been included for
a given counter, not just the total.
On Tue, Sep 11, 2018 at 15:57 Daniel Nelson ***@***.******@***.***>> wrote:
@m82labs <https://github.com/m82labs> One last question, all the changes
are backwards compatible? New fields and tags are okay but are there any
renames or type changes?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#4642 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AIS1dD-UmbexEcTBw6u9dnvYH5jHR1Nyks5uaBWwgaJpZM4WdXUk>
.
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub<https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Finfluxdata%2Ftelegraf%2Fpull%2F4642%23issuecomment-420482962&data=02%7C01%7Cdenzilr%40microsoft.com%7C06b7da215ba3430ff17608d61850d7cd%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636723132831213011&sdata=eiPE1Fri%2FCuNSym%2Bb7VjEQsvIKMSWlwmFOwJMBEg%2Fho%3D&reserved=0>, or mute the thread<https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fnotifications%2Funsubscribe-auth%2FAL-s1NQPhUof_OZUp7ngKwAS3CMFs8JRks5uaGZBgaJpZM4WdXUk&data=02%7C01%7Cdenzilr%40microsoft.com%7C06b7da215ba3430ff17608d61850d7cd%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636723132831369270&sdata=Tapsbo98I1N6Z6igIgqJZ%2BOrqxhhV7pR5Zec3pCzDsY%3D&reserved=0>.
|
|
Users always have the option to trim out unwanted data with tagpass, tagdrop, fieldpass, etc so thats no problem. Thanks again @m82labs and @denzilribeiro |
|
The readme for the plugin specifies grant VIEW SERVER STATE and VIEW ANY DEFINITION and that doesn't work on Azure. Can you please share the privileges you granted to the telegraf account? Or ideally update the readme for the plugin. |
|
@erik-wramner can you try granting VIEW DATABASE STATE. I'll try to get something set up to test today, but that might be the permission needed. |
|
The Azure SQL DB (not managed instance) plugin part has to be reworked some. The permissions do work on Azure SQL managed instance aka - https://docs.microsoft.com/en-us/azure/sql-database/sql-database-managed-instance
To be honest the Azure SQL DB part of plugin has to be rethought as a whole and queries be made conditional based on version type, different dashboards to visualize the "per database" metrics I can help Mark with that but anytime after next 2 weeks, address that perhaps in a different PR. |
|
I would like to reopen this issue, please. Perhaps it is only documentation, but the plugin is still not working. I tried connecting directly to a database with a user with VIEW DATABASE STATE and the plugin crashes. First it complains about permissions:
Then it crashes: `panic: runtime error: invalid memory address or nil pointer dereference goroutine 24 [running]: This is with the build telegraf-1.8.0~rc2_windows_amd64 which should contain the fix? Please update the documentation or at least this issue with a working connection string for a user with well-defined permissions and (if necessary) what edition of SQL Database that is supported. I'm not using managed instance, which is still in preview. |
|
@erik-wramner Can you open the panic as a new issue if is different from the stack on #4222? I reopened 4222. |
|
The stacks are the same. Is it possible that telegraf-1.8.0~rc2_windows_amd64 contains the old plugin? It is a new build, only a few days old, but it is odd that the line numbers are the same? |
|
@erik-wramner I don't think its possible, the changes in 1.8 mostly just modify the queries later in the file. |
closes #4222
closes #4597
Required for all PRs:
Changes
Background Writer pages/secwhich tracks indirect checkpointsdenisenkomSQL package, prior to the change this plugin would not work properly with managed instancesThe majority of these changes came from Denzil Ribeiro from Microsoft, so big thanks to Denzil!