Adding more option to enable required mongo stats to monito #6021

Nitesh-vaidyanath · 2023-12-26T00:03:37Z

PR Description

Which issue(s) this PR fixes

Notes to the Reviewer

PR Checklist

CHANGELOG.md updated
Documentation added
Tests updated
[] Config converters updated

CLAassistant · 2023-12-26T00:03:43Z

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.

Nitesh Vaidyanath seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

marctc · 2023-12-27T14:29:37Z

pkg/integrations/mongodb_exporter/mongodb_exporter.go

 }

+var collectAll = true


Is there any reason to make this variable global?

marctc · 2023-12-27T14:32:01Z

pkg/integrations/mongodb_exporter/mongodb_exporter.go

@@ -17,15 +17,29 @@ import (

 var DefaultConfig = Config{
 	DirectConnect: true,
+	EnableDBStats: false,


wouldn't be better to have all of this enabled by default instead of setting them to true later (from line 92).

When collectAll is enabled all the stats are enabled by default. If someone wants to enable specific stats they can just enable that flag, which in turn disables collectAll.

Maybe I'm missing something, but I still don't understand why these flags are not enabled by default and we need a collectAll variable (which is true by default ).

Yeah it is bit confussing but setting collectAll to true and enabling all other other flags doesn't make any differnce. In the comment i added for the sake of understanding, collectAll=ture is equaliant to enabling all the other flags however, if we enable any one or more flags it will diasable collectAll and other flags too.

If it's confusing for you, it will be confusing for the rest of users. Would you mind refactor the configuration to be easier to understand?

I think I understand the point that, because CollectAll was hardcoded to true, the new flags won't have any effect unless we expose the flag and users manually disable it.

In order to mirror the actual collector's logic as closely as possible and avoid misunderstanding from users, I would suggest fully exposing CollectAll as a new flag, and setting it to true in DefaultConfig (as @marctc suggested).

Rationale: most users won't need that level of granularity, but if they do, they can disable CollectAll and set each desired flag individually.

We should also make sure that we are exposing all the flags that would be enabled by CollectAll (afaik we are missing some).

This is also a potential argument for not exposing any flags at all: if the collector dependency is updated and some new flags are added and missed as part of the update, users who manually set the list of collectors to enable may miss the new ones, so there is some maintenance cost involved in providing such level of granularity.

Let me refactor and push my changes.

marctc · 2023-12-27T14:33:20Z

docs/sources/static/configuration/integrations/mongodb_exporter-config.md

@@ -79,6 +79,34 @@ Besides that, there's not much to configure. Please refer to the full reference
  # MongoDB node connection URL, which must be in the [`Standard Connection String Format`](https://docs.mongodb.com/manual/reference/connection-string/#std-label-connections-standard-connection-string-format)
  [mongodb_uri: <string>]

+  # By default all the below stats will be


This explanation should be also added to flow component doc. But if we change the default behavior, we can remove them altogether.

Hi @marctc, there is no change in default behavior. I have just added more configurable parameter.

Yes i have already added this to docs/sources/flow/reference/components/prometheus.exporter.mongodb.md

By default behavior I meant my comment in https://github.com/grafana/agent/pull/6021/files#r1437078040. The component docs specifies that those flags are disabled by default but then the code does the opposite (as it reflected here).

marctc · 2024-01-04T09:42:43Z

CHANGELOG.md

@@ -2317,6 +2317,8 @@ v0.24.0 (2022-04-07)

 ### Features

+- Adding more option to enable required mongo stats to monitor. (@Nitesh-vaidyanath)


This changelog entry is placed in a old release section. Please, move it to unreleased section.

marctc

Please see comments. Once comments are addressed the PR would be ready to be merged.

github-actions · 2024-02-10T00:09:46Z

This PR has not had any activity in the past 30 days, so the needs-attention label has been added to it.
If you do not have enough time to follow up on this PR or you think it's no longer relevant, consider closing it.
The needs-attention label signals to maintainers that something has fallen through the cracks. No action is needed by you; your PR will be kept open and you do not have to respond to this comment. The label will be removed the next time this job runs if there is new activity.
Thank you for your contributions!

tpaschalis · 2024-02-20T13:37:03Z

@Nitesh-vaidyanath I think we're on a good path, do you have the time to address @marctc's feedback here?

clayton-cornell

Some doc input

clayton-cornell · 2024-02-20T23:26:45Z

docs/sources/flow/reference/components/prometheus.exporter.mongodb.md

+| `enable_db_stats`            | `boolean` | Enable mongo database stats | false   | no       |
+| `enable_diagnostic_data`     | `boolean` | Enable mongo diagonstic data | false   | no       |
+| `enable_replicaset_status`   | `boolean` | Enable mongo replica set status stats | false   | no       |
+| `enable_top_metrics`         | `boolean` | Enable mongo top metrics | false   | no       |
+| `enable_index_stats`         | `boolean` | Enable mongo index stats | false   | no       |
+| `enable_coll_stats`          | `boolean` | Enable mongo collection stats | false   | no       |


clayton-cornell · 2024-02-20T23:27:36Z

docs/sources/static/configuration/integrations/mongodb_exporter-config.md

+  # Mongo exporter variable EnableDBStats 
+  [enable_db_stats: <boolean> | default = false]
+  # Mongo exporter variable  EnableDiagnosticData
+  [enable_diagnostic_data: <boolean> | default = false]
+  # Mongo exporter variable EnableReplicasetStatus
+  [enable_replicaset_status: <boolean> | default = false]
+  # Mongo exporter variable EnableTopMetrics
+  [enable_top_metrics: <boolean> | default = false]
+  # Mongo exporter variable EnableIndexStats
+  [enable_index_stats: <boolean> | default = false]
+  # Mongo exporter variable EnableCollStats


Suggested change

# Mongo exporter variable EnableDBStats

[enable_db_stats: <boolean> | default = false]

# Mongo exporter variable EnableDiagnosticData

[enable_diagnostic_data: <boolean> | default = false]

# Mongo exporter variable EnableReplicasetStatus

[enable_replicaset_status: <boolean> | default = false]

# Mongo exporter variable EnableTopMetrics

[enable_top_metrics: <boolean> | default = false]

# Mongo exporter variable EnableIndexStats

[enable_index_stats: <boolean> | default = false]

# Mongo exporter variable EnableCollStats

# MongoDB exporter variable EnableDBStats

[enable_db_stats: <boolean> | default = false]

# MongoDB exporter variable EnableDiagnosticData

[enable_diagnostic_data: <boolean> | default = false]

# MongoDB exporter variable EnableReplicasetStatus

[enable_replicaset_status: <boolean> | default = false]

# MongoDB exporter variable EnableTopMetrics

[enable_top_metrics: <boolean> | default = false]

# MongoDB exporter variable EnableIndexStats

[enable_index_stats: <boolean> | default = false]

# MongoDB exporter variable EnableCollStats

clayton-cornell · 2024-02-20T23:28:33Z

CHANGELOG.md

@@ -2317,6 +2317,8 @@ v0.24.0 (2022-04-07)

 ### Features

+- Adding more option to enable required mongo stats to monitor. (@Nitesh-vaidyanath)


Suggested change

- Adding more option to enable required mongo stats to monitor. (@Nitesh-vaidyanath)

- Adding more option to enable required MongoDB stats to monitor. (@Nitesh-vaidyanath)

We also need to correct the MongoDB name to match their product name/capitalization

Nitesh Vaidyanath added 2 commits December 25, 2023 15:52

#6020 - Adding more option to enable required mongo stats to monitor

c654e4c

fixed lined allignment

6df318b

Nitesh-vaidyanath requested a review from clayton-cornell as a code owner December 26, 2023 00:03

Nitesh Vaidyanath added 3 commits December 25, 2023 16:20

fixed lined allignment

0cb14a1

added to changelog

11e042b

fixed typo

235ca4f

marctc reviewed Dec 27, 2023

View reviewed changes

marctc reviewed Jan 4, 2024

View reviewed changes

marctc requested changes Jan 4, 2024

View reviewed changes

github-actions bot added the needs-attention An issue or PR has been sitting around and needs attention. label Feb 10, 2024

clayton-cornell reviewed Feb 20, 2024

View reviewed changes

github-actions bot removed the needs-attention An issue or PR has been sitting around and needs attention. label Feb 23, 2024

rfratto added variant/static Related to Grafana Agent Static. variant/flow Relatd to Grafana Agent Flow. enhancement New feature or request labels Apr 9, 2024

Nitesh-vaidyanath mentioned this pull request Dec 26, 2023

Adding more option to enable required mongo stats to monitor grafana/alloy#284

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding more option to enable required mongo stats to monito #6021

Adding more option to enable required mongo stats to monito #6021

Nitesh-vaidyanath commented Dec 26, 2023 •

edited

Loading

CLAassistant commented Dec 26, 2023

marctc Dec 27, 2023

marctc Dec 27, 2023

Nitesh-vaidyanath Jan 3, 2024

marctc Jan 3, 2024 •

edited

Loading

Nitesh-vaidyanath Jan 4, 2024

marctc Jan 4, 2024

jcreixell Jan 4, 2024

Nitesh-vaidyanath Jan 10, 2024

marctc Dec 27, 2023

Nitesh-vaidyanath Jan 3, 2024

Nitesh-vaidyanath Jan 3, 2024

marctc Jan 3, 2024

marctc Jan 4, 2024

marctc left a comment

github-actions bot commented Feb 10, 2024

tpaschalis commented Feb 20, 2024

clayton-cornell left a comment

clayton-cornell Feb 20, 2024

clayton-cornell Feb 20, 2024

clayton-cornell Feb 20, 2024

		@@ -2317,6 +2317,8 @@ v0.24.0 (2022-04-07)

		### Features

		- Adding more option to enable required mongo stats to monitor. (@Nitesh-vaidyanath)

Adding more option to enable required mongo stats to monito #6021

Are you sure you want to change the base?

Adding more option to enable required mongo stats to monito #6021

Conversation

Nitesh-vaidyanath commented Dec 26, 2023 • edited Loading

PR Description

Which issue(s) this PR fixes

Notes to the Reviewer

PR Checklist

CLAassistant commented Dec 26, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marctc Jan 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marctc left a comment

Choose a reason for hiding this comment

github-actions bot commented Feb 10, 2024

tpaschalis commented Feb 20, 2024

clayton-cornell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Nitesh-vaidyanath commented Dec 26, 2023 •

edited

Loading

marctc Jan 3, 2024 •

edited

Loading