-
Notifications
You must be signed in to change notification settings - Fork 11.6k
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
Unified Storage: Adds sql metrics #87651
Unified Storage: Adds sql metrics #87651
Conversation
…rom postgres connection string format
@@ -69,7 +71,7 @@ func (db *EntityDB) GetEngine() (*xorm.Engine, error) { | |||
} | |||
|
|||
connectionString := fmt.Sprintf( | |||
"user='%s' password='%s' host='%s' port='%s' dbname='%s' sslmode='%s'", // sslcert='%s' sslkey='%s' sslrootcert='%s'", | |||
"user=%s password=%s host=%s port=%s dbname=%s sslmode=%s", // sslcert='%s' sslkey='%s' sslrootcert='%s'", |
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.
Had to change this so things can run locally. This issue is fixed in this PR
// register the go_sql_stats_connections_* metrics | ||
if err := prometheus.Register(sqlstats.NewStatsCollector("unified_storage", db.engine.DB().DB)); err != nil { | ||
db.log.Warn("Failed to register unified storage sql stats collector", "error", err) | ||
} | ||
|
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.
Should this be up in the block where we create the new connection? Or do we need it even when we are piggybacking on the main grafana connection?
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.
If we're piggybacking on the grafana connection, then this function should return right away bc of this
if db.engine != nil {
return db.engine, nil
}
at the beginning.
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.
nope, check out lines 128-134
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 check for whether we've already set up db.engine is just a shortcut so you can call GetEngine again and it'll return the engine we configured the first time.
Since we use a separate db engine for US, we weren't getting any db metrics.