-
Notifications
You must be signed in to change notification settings - Fork 491
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
cmd/scollector: add NETDataProviderForSqlServer metrics #2225
Conversation
tags := opentsdb.TagSet{"name": name, "id": "0"} | ||
// Not a 100% on counter / gauge here, may see some wrong after collecting more data. PerSecond being a counter is expected | ||
// however since this is a PerfRawData table | ||
Add(&md, "dotnet.sqlprov.hard_connects", v.HardConnectsPerSecond, tags, metadata.Counter, metadata.Connection, descWinDotNetSQLHardConnectsPerSecond) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW, I'd just call this dotnet.sql.<thing>
const ( | ||
descWinDotNetSQLHardConnectsPerSecond = "A counter of the number of actual connections per second that are being made to servers." | ||
descWinDotNetSQLHardDisconnectsPerSecond = "A counter of the number of actual disconnects per second that are being made to servers." | ||
descWinDotNetSQLNumberOfActiveConnectionPoolGroups = "The number of unique connection strings." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"The number of unique connection pool groups." - it's not exactly 1:1 with Connection strings, it just happens to be for us. For example, when using AD auth and impersonation it'd be 1 connection string but 1 pool per identity.
descWinDotNetSQLNumberOfActiveConnectionPools = "The number of active connection pools." | ||
descWinDotNetSQLNumberOfActiveConnections = "The number of connections currently in-use." | ||
descWinDotNetSQLNumberOfFreeConnections = "The number of connections currently available for use." | ||
descWinDotNetSQLNumberOfInactiveConnectionPoolGroups = "The number of unique connection strings waiting for pruning." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"The number of unique connection pool groups waiting for pruning." (see above comment)
descWinDotNetSQLNumberOfInactiveConnectionPools = "The number of inactive connection pools." | ||
descWinDotNetSQLNumberOfNonPooledConnections = "The number of connections that are not using connection pooling." | ||
descWinDotNetSQLNumberOfPooledConnections = "The number of connections that are managed by the connection pooler." | ||
descWinDotNetSQLNumberOfReclaimedConnections = "The number of connections we reclaim from GCed external connections." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would phrase these as: "The number of connections reclaimed from GCed external connections."
descWinDotNetSQLNumberOfPooledConnections = "The number of connections that are managed by the connection pooler." | ||
descWinDotNetSQLNumberOfReclaimedConnections = "The number of connections we reclaim from GCed external connections." | ||
descWinDotNetSQLNumberOfStasisConnections = "The number of connections currently waiting to be made ready for use." | ||
descWinDotNetSQLSoftConnectsPerSecond = "A counter of the number of connections we get from the pool per second." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"The number of connections opened from the pool per second."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewing for descriptions - they're not far off now, but put suggestions for improvements inline.
descWinDotNetSQLNumberOfReclaimedConnections = "The number of connections we reclaim from GCed external connections." | ||
descWinDotNetSQLNumberOfStasisConnections = "The number of connections currently waiting to be made ready for use." | ||
descWinDotNetSQLSoftConnectsPerSecond = "A counter of the number of connections we get from the pool per second." | ||
descWinDotNetSQLSoftDisconnectsPerSecond = "A counter of the number of connections we return to the pool per second." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"The number of connections closed and returned to the pool per second."
} | ||
var md opentsdb.MultiDataPoint | ||
// We add the values of multiple pools that share a PID, this could cause some counter edge cases | ||
pidToRows := make(map[string][]int) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please explain what the counters represent in the value arrays. Are they fixed length or variable?
// is skipped when adding metrics | ||
for _, idx := range rows[1:] { | ||
skipIndex[idx] = true | ||
dst[rows[0]].HardDisconnectsPerSecond += dst[idx].HardDisconnectsPerSecond |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's store dst[rows[0]] as a variable to clean up the repetition here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And probably dst[idx] too.
} | ||
var name string | ||
// Extract PID from the Name field, which is odd in this class | ||
m := dotNetSQLPIDRegex.FindStringSubmatch(v.Name) // Index to remote actuator pylons to check for review quality |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good one.
} | ||
pidToRows[m[1]] = append(pidToRows[m[1]], i) | ||
} | ||
skipIndex := make(map[int]bool) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please describe the purpose of this, and what true/false indicate
No description provided.