WIP: Implement mssql base counters #463
Conversation
secustor
force-pushed
the
implement-mssql-base-counters
branch
from
8beba59 to
9d481c2
Compare
collector/mssql.go
Outdated
| @@ -662,6 +666,12 @@ func NewMSSQLCollector() (Collector, error) { | |||
| []string{"instance"}, | |||
| nil, | |||
| ), | |||
| AccessMethodsWorktablesFromCacheRatio_Base: prometheus.NewDesc( | |||
| prometheus.BuildFQName(Namespace, subsystem, "accessmethods_worktables_from_cache_ratio_lookups"), | |||
There was a problem hiding this comment.
Skip the _ratio here, since it is not lookups on the ratio
There was a problem hiding this comment.
Also, the metric accessmethods_worktables_from_cache_ratio should probably be accessmethods_worktables_from_cache_hits, right?
There was a problem hiding this comment.
(The above seems to repeat for all the new changes)
There was a problem hiding this comment.
The bases have no meaning on itself if you go by the Microsoft documentation.
Better said MS is not sharing what they are in their documentation.
Because of this I would prefer the naming style of @sqlkabouter .
We can't be sure here as we have to go with the documentation of Microsoft.
There was a problem hiding this comment.
I'm sorry to be a bit contrarian here, but while I appreciate that the documentation is a bit vague and the current naming wrong, I'm not sure I'm keen on merging something which introduces more unclarity. The name here is pretty bad in that regard, in my opinion.
Looking at the docs you linked:
The ratio is the total number of cache hits divided by the total number of cache lookups over the last few thousand page accesses.
By that definition, it seems pretty clear to me that we get the total lookups in the base, and total hits in the "non-base" counter? Are there other reasonable interpretations I'm missing?
There was a problem hiding this comment.
It seems to be the case after testing with MS monitoring tools.
Our DB team will hopefully confirm with this Snapshot that the results using Prometheus+WMI_Exporter and MS tools report the same stats.
|
@carlpett Can give me feedback what is not working in the pipeline? |
|
@secustor It seems it didn't even try to build... And Github's usability team could do better here I think - I need to hover over the failing check to get the text "AppVeyor was unable to build non-mergeable pull request". So if you resolve the conflict it will build. |
secustor
force-pushed
the
implement-mssql-base-counters
branch
from
eed771c to
b2f521d
Compare
secustor
changed the title
|
I hope you can see the build logs this time? If not, the failing part is linting, specifically that the files are not |
secustor
force-pushed
the
implement-mssql-base-counters
branch
from
9d1b34e to
696c447
Compare
secustor
force-pushed
the
implement-mssql-base-counters
branch
from
696c447 to
6dad58f
Compare
|
Thank you so very much @secustor! 👏 |
…ssql-base-counters WIP: Implement mssql base counters
Resolves #250
Continuation of #352 as this contribution seems to be abandoned.