Add collector for database/sql#DBStats #866

johejo · 2021-05-12T11:44:57Z

Signed-off-by: Mitsuo Heijo [email protected]

This is redo of #848

Signed-off-by: Mitsuo Heijo <[email protected]>

beorn7

Looks good in general, but I have a few requests for the code structures and the naming.

beorn7 · 2021-05-14T17:54:39Z

prometheus/collectors/dbstats_collector.go

+	var fqName func(name string) string
+	if opts.DriverName == "" {
+		fqName = func(name string) string {
+			return "go_db_stats_" + name


What do you think about go_sql_ as prefix rather than go_db_stats_? The package it's coming from is called sql. And that kind of says everything. Obviously, it's about a database, and obviously, metrics ar "stats". go_sql_open_connections reads nicely, doesn't it?

beorn7 · 2021-05-14T18:00:55Z

prometheus/collectors/dbstats_collector.go

+type DBStatsCollectorOpts struct {
+	// DriverName holds the name of driver.
+	// It will not used for empty strings.
+	DriverName string


How about just calling this Name or DBName? A user could create different *sql.DB instances with the same driver.

In different news: Do you anticipate that we need more fields in this options struct? An options struct is great for later non-breaking additions, but if those additions never happen, it's just overhead. I cannot think about anything needed here right now, so I would tend towards just having a dbName argument on the NewDBStatsCollector constructor.

beorn7 · 2021-05-14T18:02:14Z

prometheus/collectors/dbstats_collector.go

+		}
+	} else {
+		fqName = func(name string) string {
+			return "go_" + opts.DriverName + "_db_stats_" + name


I'm pretty sure the driver name should not be part of the metric name. It should be a label, perhaps called db_name.

That label has to be a const label when creating the descriptors, see comment below.

beorn7 · 2021-05-14T18:04:09Z

prometheus/collectors/dbstats_collector.go

+		maxOpenConnections: prometheus.NewDesc(
+			fqName("max_open_connections"),
+			"Maximum number of open connections to the database.",
+			nil, nil,


To add the const label as suggested above, this line needs to be

nil, prometheus.Labels{"db_name": opts.DriverName},

beorn7 · 2021-05-14T18:12:36Z

prometheus/collectors/dbstats_collector_go115.go

@@ -0,0 +1,129 @@
+// Copyright 2021 The Prometheus Authors


I would call this file dbstats_collector_pre_1.15.go.

Also, I think, you can avoid duplicating most of the code here.

For example, you could have the Desc for MaxIdleTimeClosed in any case, but do not Describe it and do not Collect the corresponding metric. Instead, have additional method calls there DescribeGo115Stats and CollectGo115Stats, and then just have those methods here (as empty), and the proper methods in a separate file with // +build go1.15 as a tag.

beorn7 · 2021-05-14T18:14:49Z

prometheus/collectors/dbstats_collector_go115.go

+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+//go:build !go1.15


What is this for? Isn't the directive in the next line the proper one?

Signed-off-by: Mitsuo Heijo <[email protected]>

beorn7

Thanks.

You didn't implement my suggestion about reducing the code duplication for different Go versions. I think it's easiest if we merge this now and then I'll send you a PR to review.

johejo · 2021-05-20T20:35:15Z

Sorry, I couldn't fully understand and implement your idea. Looking forward to your additional PR.

Add collector for database/sql#DBStats

c0be446

Signed-off-by: Mitsuo Heijo <[email protected]>

beorn7 self-requested a review May 12, 2021 11:54

beorn7 requested changes May 14, 2021

View reviewed changes

improve metrics structure

c20bd79

Signed-off-by: Mitsuo Heijo <[email protected]>

johejo force-pushed the add_dbstats_collector branch from 7f6e05e to c20bd79 Compare May 19, 2021 17:16

johejo requested a review from beorn7 May 19, 2021 17:25

beorn7 approved these changes May 20, 2021

View reviewed changes

beorn7 merged commit a66da1d into prometheus:master May 20, 2021

Ehco1996 mentioned this pull request Sep 17, 2021

Database connection number monitoring pingcap/dm#2144

Open

johejo mentioned this pull request Mar 18, 2022

Remove workaround for pre go1.15 #1010

Merged

This was referenced May 19, 2022

metrics support k3s-io/kine#119

Closed

Introduce metrics k3s-io/kine#121

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add collector for database/sql#DBStats #866

Add collector for database/sql#DBStats #866

johejo commented May 12, 2021

beorn7 left a comment

beorn7 May 14, 2021

beorn7 May 14, 2021

beorn7 May 14, 2021

beorn7 May 14, 2021

beorn7 May 14, 2021

beorn7 May 14, 2021

beorn7 left a comment

johejo commented May 20, 2021

Add collector for database/sql#DBStats #866

Add collector for database/sql#DBStats #866

Conversation

johejo commented May 12, 2021

beorn7 left a comment

Choose a reason for hiding this comment

beorn7 May 14, 2021

Choose a reason for hiding this comment

beorn7 May 14, 2021

Choose a reason for hiding this comment

beorn7 May 14, 2021

Choose a reason for hiding this comment

beorn7 May 14, 2021

Choose a reason for hiding this comment

beorn7 May 14, 2021

Choose a reason for hiding this comment

beorn7 May 14, 2021

Choose a reason for hiding this comment

beorn7 left a comment

Choose a reason for hiding this comment

johejo commented May 20, 2021