[receiver\sqlserver] Fix SQL Server metrics from gauge to monotonic sum #9520
Conversation
StefanKurek
force-pushed
the
sqlserver_receiver
branch
from
47541c6
to
2990927
Compare
|
@dashpole this is a pretty small PR. If you could take a look I'd be very appreciative. |
There was a problem hiding this comment.
This looks reasonable to me, but I'm not familiar with sql server at all. Can you link to docs for where the data is being retrieved from (if they exist?). This component doesn't seem to have an entry in CODEOWNERS, which is unfortunate. @djaglowski is the CODEOWNERS for other sql-related components, maybe he has context to review?
@dashpole Hello. https://docs.microsoft.com/en-us/sql/relational-databases/performance-monitor/sql-server-databases-object?view=sql-server-ver15 is the best I can find for documentation (specifically log growths and log shrinks). For context, when pulled from the monitoring tables themselves, most of the data is is in counter form. The same data pulled with windows performance counters (which is what the first pass of this receiver uses exclusively), almost every metric is a gauge instead. I assumed too much about these two metrics when initially creating the receiver, but after generating load and looking at non-zero values I saw that they were indeed counters. I should have been tipped off because these two counter names (Log Growths & Log Shrinks) do not include a "/sec" in their name like most of the counters. I don't think @djaglowski will have any info outside of what I just described (and he is actually on vacation until next week I believe). Thanks again for looking! |
| gauge: | ||
| sum: | ||
| monotonic: true | ||
| aggregation: cumulative | ||
| value_type: double |
There was a problem hiding this comment.
@bogdandrutu it's the way it is presented from the windows performance counters. it might be able to be changed to an int with some research, but i'd say that's outside the scope of this bug fix.
There was a problem hiding this comment.
@StefanKurek will you please create an issue to track this?
| gauge: | ||
| sum: | ||
| monotonic: true | ||
| aggregation: cumulative | ||
| value_type: double |
There was a problem hiding this comment.
See above comment
StefanKurek
changed the title
There was a problem hiding this comment.
@StefanKurek, I've opened #9600 to declare you and I as code owners of this component. Otherwise, I think we should capture the double/int question and will need to address it before the component is graduated to beta.
| gauge: | ||
| sum: | ||
| monotonic: true | ||
| aggregation: cumulative | ||
| value_type: double |
There was a problem hiding this comment.
@StefanKurek will you please create an issue to track this?
Description:
Fix bug where transaction log shrinks and growths were improperly marked as gauges when they are in fact monotonic sums.
Issue: #9522
Testing: Manually ran and verified that changed metrics came back as monotonic sums instead of gauges.
Documentation: Updated documentation to reflect metric changes