metricbeat/module/mongodb/collstats: Add extra collstats metrics #42171
Conversation
stefans-elastic
added
enhancement
Team:Obs-InfraObs
botelastic
bot
added
needs_team
|
This pull request does not have a backport label.
To fixup this pull request, you need to add the backport labels for the needed
|
|
|
mergify
bot
added
the
backport-8.x
|
Here are a few suggestions: diff --git a/metricbeat/module/mongodb/collstats/_meta/fields.yml b/metricbeat/module/mongodb/collstats/_meta/fields.yml
index 27c4f989a7..a4a09f8d09 100644
--- a/metricbeat/module/mongodb/collstats/_meta/fields.yml
+++ b/metricbeat/module/mongodb/collstats/_meta/fields.yml
@@ -117,19 +117,19 @@
- name: stats.avgObjSize
type: long
description: >
- The average size of an object in the collection.
+ The average size of an object in the collection (in bytes).
- name: stats.storageSize
type: long
description: >
- The total amount of storage allocated to this collection for document storage.
+ The total amount of storage allocated to this collection for document storage (in bytes).
- name: stats.totalIndexSize
type: long
description: >
- The total size of all indexes.
+ The total size of all indexes (in bytes).
- name: stats.totalSize
type: long
description: >
- The sum of the storageSize and totalIndexSize.
+ The sum of the storageSize and totalIndexSize (in bytes).
- name: stats.max
type: long
description: >
diff --git a/metricbeat/module/mongodb/collstats/collstats.go b/metricbeat/module/mongodb/collstats/collstats.go
index 64f345c8d2..c760845d0f 100644
--- a/metricbeat/module/mongodb/collstats/collstats.go
+++ b/metricbeat/module/mongodb/collstats/collstats.go
@@ -90,7 +90,7 @@ func (m *Metricset) Fetch(reporter mb.ReporterV2) error {
totals, ok := result["totals"].(map[string]interface{})
if !ok {
- return errors.New("collection 'totals' are not a map")
+ return errors.New("collection 'totals' is not a map")
}
if err = res.Err(); err != nil {
@@ -147,9 +147,13 @@ func (m *Metricset) Fetch(reporter mb.ReporterV2) error {
func fetchCollStats(client *mongo.Client, dbName, collectionName string) (map[string]interface{}, error) {
db := client.Database(dbName)
- colStats := db.RunCommand(context.Background(), bson.M{"collStats": collectionName})
+ collStats := db.RunCommand(context.Background(), bson.M{"collStats": collectionName})
+ if err := collStats.Err(); err != nil {
+ return nil, fmt.Errorf("collStats command failed: %w", err)
+ }
+
var statsRes map[string]interface{}
- if err := colStats.Decode(&statsRes); err != nil {
+ if err := collStats.Decode(&statsRes); err != nil {
return nil, fmt.Errorf("could not decode mongo response for database=%s, collection=%s: %w", dbName, collectionName, err)
}
diff --git a/metricbeat/module/mongodb/collstats/data.go b/metricbeat/module/mongodb/collstats/data.go
index cef81df7c3..e9600fb83e 100644
--- a/metricbeat/module/mongodb/collstats/data.go
+++ b/metricbeat/module/mongodb/collstats/data.go
@@ -30,9 +30,12 @@ func eventMapping(key string, data mapstr.M) (mapstr.M, error) {
return nil, err
}
+ // NOTE: splitKey handles the case where the collection can have "." in the name
+ database, collection := names[0], names[1]
+
event := mapstr.M{
- "db": names[0],
- "collection": names[1],
+ "db": database,
+ "collection": collection,
"name": key,
"total": mapstr.M{
"time": mapstr.M{
@@ -111,11 +114,11 @@ func mustGetMapStrValue(m mapstr.M, key string) interface{} {
}
func splitKey(key string) ([]string, error) {
- names := strings.SplitN(key, ".", 2)
+ dbColl := strings.SplitN(key, ".", 2)
- if len(names) < 2 {
+ if len(dbColl) < 2 {
return nil, errors.New("collection name invalid")
}
- return names, nil
+ return dbColl, nil
}
diff --git a/metricbeat/module/mongodb/collstats/data_test.go b/metricbeat/module/mongodb/collstats/data_test.go
index a921d14d72..eefd800c21 100644
--- a/metricbeat/module/mongodb/collstats/data_test.go
+++ b/metricbeat/module/mongodb/collstats/data_test.go
@@ -21,7 +21,7 @@ package collstats
import (
"encoding/json"
- "io/ioutil"
+ "os"
"testing"
"github.com/stretchr/testify/assert"
@@ -30,8 +30,7 @@ import (
)
func TestEventMapping(t *testing.T) {
-
- content, err := ioutil.ReadFile("./_meta/test/input.json")
+ content, err := os.ReadFile("./_meta/test/input.json")
assert.NoError(t, err)
data := mapstr.M{} |
shmsr
changed the title
|
Also, can you also write a better commit message; helps in future when we need to revisit the changes and to know what and why of the change. |
|
But yes, rest looks good! |
| "totalIndexSize": mustGetMapStrValue(data, "stats.totalIndexSize"), | ||
| "totalSize": mustGetMapStrValue(data, "stats.totalSize"), | ||
| "max": mustGetMapStrValue(data, "stats.max"), | ||
| "nindexes": mustGetMapStrValue(data, "stats.nindexes"), |
There was a problem hiding this comment.
Any reason to not add fields like -- capped?
Also, see the list here: https://github.com/DataDog/integrations-core/blob/bef0a2f2971ff01176689794aaa7eb0bb0b37a9e/mongo/datadog_checks/mongo/metrics.py#L140
If we are getting them, can we please add?
There was a problem hiding this comment.
There are quite a lot of fileds which we get here @shmsr .
We planned to list down the most useful one.
| MetricSetFields: event, | ||
| }) | ||
| wg.Add(1) | ||
| go func(eventReporter mb.ReporterV2, mongoClient *mongo.Client, group string) { |
There was a problem hiding this comment.
Also what's the rationale behind using goroutines here? Also, even if we are keeping it can be do bounded concurrency. Dont wanna fire too many queries and burden the customer's MongoDB server.
sem := make(chan struct{}, 10) // Limit concurrent operations
for group, info := range totals {
sem <- struct{}{} // Acquire
go func() {
defer func() { <-sem }() // Release
// Existing goroutine code
}()
}
I mean, can we add semaphore or worker pool to limit the concurrency?
There was a problem hiding this comment.
TIP: Also, without bounded concurrency, errgroup is cleaner way to implement this: https://pkg.go.dev/golang.org/x/sync/errgroup
There was a problem hiding this comment.
Agree to this:
For large datasets, the number of goroutines can overwhelm the system. Let's use workerpool to limit the concurrency.
There was a problem hiding this comment.
I've replaced WaitGroup with errgroup. Btw, I set goroutins limit to 10 like was suggested in the example. Is 10 a good limit or should I change it to something else?
| MetricSetFields: event, | ||
| }) | ||
| wg.Add(1) | ||
| go func(eventReporter mb.ReporterV2, mongoClient *mongo.Client, group string) { |
There was a problem hiding this comment.
Agree to this:
For large datasets, the number of goroutines can overwhelm the system. Let's use workerpool to limit the concurrency.
| return | ||
| } | ||
|
|
||
| collStats, err := fetchCollStats(mongoClient, names[0], names[1]) |
There was a problem hiding this comment.
Better naming for names[0], names[1] please
|
@ishleenk17 @shmsr thank you for your reviews! |
Proposed commit message
[metricbeats][mongodb] handle extra collstats metrics
Checklist
CHANGELOG.next.asciidocorCHANGELOG-developer.next.asciidoc.Disruptive User Impact
Author's Checklist
How to test this PR locally
Related issues
Use cases
Screenshots
Logs