SQL Perfmon counters - synced queries from v2 to all db types by avinash-nigam · Pull Request #8393 · influxdata/telegraf

avinash-nigam · 2020-11-12T08:28:25Z

Required for all PRs:

Signed CLA.
Associated README.md updated.
Has appropriate unit tests.

Includes changes :

replicated perfmon counters list from v2 query to queries across all database types

Merging original master to forked master

denzilribeiro

Is there someone that needs the counter below? There are hardly any customers using buffer pool extensions. If not needed please remove that and will aprrove.
Extension free pages | Total number of free cache pages in the buffer pool extension file.

avinash-nigam · 2020-11-18T02:26:59Z

Is there someone that needs the counter below? There are hardly any customers using buffer pool extensions. If not needed please remove that and will aprrove.
Extension free pages | Total number of free cache pages in the buffer pool extension file.

@denzilribeiro I've removed the buffer manager counters

denzilribeiro

Looks good

avinash-nigam · 2020-12-08T22:21:54Z

@ssoroka and @reimda - could you please help merge this PR as well? Thank you.

ssoroka · 2020-12-08T22:26:32Z

plugins/inputs/sqlserver/sqlqueriesV2.go

-				''Free pages'',
-				''Extension free pages'',


why do we remove these two?

These were added recently as part of PR - #8120, however these counters are rarely used (Denzil's comment above mention the same), hence were removed.

@ssoroka - any thoughts on the above? if there are no concerns, could you please help merge the same? thank you!

any chance that someone is depending on them?

Unlikely this was supposed to be added to here instead by mistake was recently added to V2.

@ssoroka your decision here. #8120 was merged on Sept. 2020 and thus was release in 1.17.

My question to @denzilribeiro is if it was intended to be added here, why do you now remove it. In general, what is the cost of keeping those two in there?

@srebhan - to be honest should have not been added as rarely ( < 0.5%) do SQL Server folks use it. Removal was more from perspective of unnecessary space, not opposed to leaving it but if 99.9% are not using the feature that enables those counster, let alone the montiroing for it it then why? But you guys' call

It's possible if someone is using it this could break a chart. I'm on the fence here, but if it's all the same to everyone let's just leave the fields in and they can always drop the ones they don't want.

@srebhan @ssoroka - Let me revert the change of removing the two counters, rest of the PR would remain as is. I'll push the change and ping back here.

@srebhan @ssoroka - The two counters' removal has been reverted. If there are no other concerns, it'd be very helpful to get this merged. Thank you.

avinash-nigam · 2020-12-15T17:08:35Z

@ssoroka and @reimda - Could you please help merge this PR if there are no concerns? Thank you.

avinash-nigam · 2021-01-05T01:35:20Z

@ssoroka and @reimda - Could you please help merge this PR if there are no concerns? Thank you.

@ssoroka and @reimda - Any thoughts on the above?

masree · 2021-01-19T04:57:35Z

@ssoroka and @reimda - Could you please help merge this PR if there are no concerns? Thank you.

@ssoroka and @reimda - Any thoughts on the above?

@ssoroka and @reimda - Can you please help merge PR if no concerns? Thank you!

denzilribeiro · 2021-01-19T15:20:12Z

@danielnelson / @ssoroka / @sjwang90 can someone help merge this or are there pending issues that need to be addressed?

sjwang90 · 2021-01-19T20:16:36Z

@denzilribeiro We'll get this reviewed and let you know if there's anything else before it gets merged. Thanks!

masree · 2021-01-24T22:16:51Z

@sjwang90 / @ssoroka Can you help take a look of this PR and merge it if no concerns? Thanks!

srebhan

I think the removal of the two properties needs to be agreed on by someone from InfluxData. Alternatively, you could leave those lines in if it is not prohibitively costly (my preferred solution).

avinash-nigam · 2021-02-04T23:14:52Z

I think the removal of the two properties needs to be agreed on by someone from InfluxData. Alternatively, you could leave those lines in if it is not prohibitively costly (my preferred solution).

@srebhan - Reverted the removal of the two counters.

srebhan

Looks good to me.

avinash-nigam · 2021-02-12T22:08:14Z

@sjwang90 @ssoroka, could you please help merge this?

…data#8393)

avinash-nigam and others added 2 commits October 29, 2020 03:19

Merge pull request #1 from influxdata/master

8d61ca4

Merging original master to forked master

synced performance counter queries from v2 to all db types

586c565

denzilribeiro suggested changes Nov 16, 2020

View reviewed changes

removed rarely used BPEX perf counters

44f3fb5

denzilribeiro approved these changes Nov 18, 2020

View reviewed changes

sjwang90 added area/sqlserver feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin labels Nov 24, 2020

ssoroka reviewed Dec 8, 2020

View reviewed changes

sjwang90 added this to the Planned milestone Dec 9, 2020

sjwang90 removed this from the Planned milestone Jan 29, 2021

srebhan reviewed Feb 3, 2021

View reviewed changes

srebhan self-assigned this Feb 3, 2021

sqlserver plugin reverted BPEX perf counters removal for older query

deff68d

srebhan approved these changes Feb 5, 2021

View reviewed changes

srebhan added the ready for final review This pull request has been reviewed and/or tested by multiple users and is ready for a final review. label Feb 5, 2021

helenosheaa approved these changes Feb 19, 2021

View reviewed changes

helenosheaa merged commit 2372db9 into influxdata:master Feb 19, 2021

arstercz pushed a commit to arstercz/telegraf that referenced this pull request Mar 5, 2023

SQL Perfmon counters - synced queries from v2 to all db types (influx…

644dce4

…data#8393)

Conversation

avinash-nigam commented Nov 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Required for all PRs:

Uh oh!

denzilribeiro left a comment

Choose a reason for hiding this comment

Uh oh!

avinash-nigam commented Nov 18, 2020

Uh oh!

denzilribeiro left a comment

Choose a reason for hiding this comment

Uh oh!

avinash-nigam commented Dec 8, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

denzilribeiro Feb 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

avinash-nigam commented Dec 15, 2020

Uh oh!

avinash-nigam commented Jan 5, 2021

Uh oh!

masree commented Jan 19, 2021

Uh oh!

denzilribeiro commented Jan 19, 2021

Uh oh!

sjwang90 commented Jan 19, 2021

Uh oh!

masree commented Jan 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

srebhan left a comment

Choose a reason for hiding this comment

Uh oh!

avinash-nigam commented Feb 4, 2021

Uh oh!

srebhan left a comment

Choose a reason for hiding this comment

Uh oh!

avinash-nigam commented Feb 12, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

avinash-nigam commented Nov 12, 2020 •

edited

Loading

denzilribeiro Feb 3, 2021 •

edited

Loading

masree commented Jan 24, 2021 •

edited

Loading