Skip to content
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

Merged
merged 4 commits into from May 13, 2024

Conversation

owensmallwood
Copy link
Contributor

@owensmallwood owensmallwood commented May 10, 2024

Since we use a separate db engine for US, we weren't getting any db metrics.

@owensmallwood owensmallwood self-assigned this May 10, 2024
@grafana-delivery-bot grafana-delivery-bot bot added this to the 11.1.x milestone May 10, 2024
@owensmallwood owensmallwood marked this pull request as ready for review May 10, 2024 18:45
@owensmallwood owensmallwood requested a review from a team as a code owner May 10, 2024 18:45
@owensmallwood owensmallwood added no-backport Skip backport of PR no-changelog Skip including change in changelog/release notes labels May 10, 2024
@@ -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'",
Copy link
Contributor Author

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

Comment on lines 143 to 147
// 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)
}

Copy link
Collaborator

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?

Copy link
Contributor Author

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.

Copy link
Collaborator

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

Copy link
Collaborator

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.

@owensmallwood owensmallwood merged commit 77686da into main May 13, 2024
12 checks passed
@owensmallwood owensmallwood deleted the owensmallwood/unified-storage-add-sql-metrics branch May 13, 2024 14:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/backend no-backport Skip backport of PR no-changelog Skip including change in changelog/release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants