-
Notifications
You must be signed in to change notification settings - Fork 486
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
Adding more option to enable required mongo stats to monito #6021
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -2317,6 +2317,8 @@ v0.24.0 (2022-04-07) | |||||
|
||||||
### Features | ||||||
|
||||||
- Adding more option to enable required mongo stats to monitor. (@Nitesh-vaidyanath) | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
We also need to correct the MongoDB name to match their product name/capitalization |
||||||
|
||||||
- Added config read API support to GrafanaAgent Custom Resource Definition. | ||||||
(@shamsalmon) | ||||||
|
||||||
|
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -39,6 +39,12 @@ Omitted fields take their default values. | |||||||||||||||||||||||||
| `direct_connect` | `boolean` | Whether or not a direct connect should be made. Direct connections are not valid if multiple hosts are specified or an SRV URI is used. | false | no | | ||||||||||||||||||||||||||
| `discovering_mode` | `boolean` | Wheter or not to enable autodiscover collections. | false | no | | ||||||||||||||||||||||||||
| `tls_basic_auth_config_path` | `string` | Path to the file having Prometheus TLS config for basic auth. Only enable if you want to use TLS based authentication. | | no | | ||||||||||||||||||||||||||
| `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 | | ||||||||||||||||||||||||||
Comment on lines
+42
to
+47
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
MongoDB node connection URI must be in the [`Standard Connection String Format`](https://docs.mongodb.com/manual/reference/connection-string/#std-label-connections-standard-connection-string-format) | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
|
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -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 | ||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This explanation should be also added to flow component doc. But if we change the default behavior, we can remove them altogether. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hi @marctc, there is no change in default behavior. I have just added more configurable parameter. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes i have already added this to docs/sources/flow/reference/components/prometheus.exporter.mongodb.md There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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). |
||||||||||||||||||||||||||||||||||||||||||||||
# collected by mongo exporter. | ||||||||||||||||||||||||||||||||||||||||||||||
# EnableDBStats=true | ||||||||||||||||||||||||||||||||||||||||||||||
# EnableCollStats=true | ||||||||||||||||||||||||||||||||||||||||||||||
# EnableTopMetrics=true | ||||||||||||||||||||||||||||||||||||||||||||||
# EnableReplicasetStatus=true | ||||||||||||||||||||||||||||||||||||||||||||||
# EnableIndexStats=true | ||||||||||||||||||||||||||||||||||||||||||||||
# EnableCurrentopMetrics=true | ||||||||||||||||||||||||||||||||||||||||||||||
# EnableProfile=true | ||||||||||||||||||||||||||||||||||||||||||||||
# EnableDiagnosticData=true | ||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||
# To disable unwanted stats and to | ||||||||||||||||||||||||||||||||||||||||||||||
# collect only required stats set parameter | ||||||||||||||||||||||||||||||||||||||||||||||
# with prefix enable to true. | ||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||
# 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 | ||||||||||||||||||||||||||||||||||||||||||||||
Comment on lines
+97
to
+107
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||
[enable_coll_stats: <boolean> | default = false] | ||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||
# Whether or not a direct connect should be made. Direct connections are not valid if multiple hosts are specified or an SRV URI is used | ||||||||||||||||||||||||||||||||||||||||||||||
[direct_connect: <boolean> | default = true] | ||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -17,15 +17,29 @@ import ( | |
|
||
var DefaultConfig = Config{ | ||
DirectConnect: true, | ||
EnableDBStats: false, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. wouldn't be better to have all of this enabled by default instead of setting them to true later (from line 92). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe I'm missing something, but I still don't understand why these flags are not enabled by default and we need a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think I understand the point that, because In order to mirror the actual collector's logic as closely as possible and avoid misunderstanding from users, I would suggest fully exposing Rationale: most users won't need that level of granularity, but if they do, they can disable We should also make sure that we are exposing all the flags that would be enabled by 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let me refactor and push my changes. |
||
EnableDiagnosticData: false, | ||
EnableReplicasetStatus: false, | ||
EnableTopMetrics: false, | ||
EnableIndexStats: false, | ||
EnableCollStats: false, | ||
} | ||
|
||
var collectAll = true | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there any reason to make this variable global? |
||
|
||
// Config controls mongodb_exporter | ||
type Config struct { | ||
// MongoDB connection URI. example:mongodb://user:[email protected]:27017/admin?ssl=true" | ||
URI config_util.Secret `yaml:"mongodb_uri"` | ||
DirectConnect bool `yaml:"direct_connect,omitempty"` | ||
DiscoveringMode bool `yaml:"discovering_mode,omitempty"` | ||
TLSBasicAuthConfigPath string `yaml:"tls_basic_auth_config_path,omitempty"` | ||
URI config_util.Secret `yaml:"mongodb_uri"` | ||
DirectConnect bool `yaml:"direct_connect,omitempty"` | ||
DiscoveringMode bool `yaml:"discovering_mode,omitempty"` | ||
EnableDBStats bool `yaml:"enable_db_stats,omitempty"` | ||
EnableDiagnosticData bool `yaml:"enable_diagnostic_data,omitempty"` | ||
EnableReplicasetStatus bool `yaml:"enable_replicaset_status,omitempty"` | ||
EnableTopMetrics bool `yaml:"enable_top_metrics,omitempty"` | ||
EnableIndexStats bool `yaml:"enable_index_stats,omitempty"` | ||
EnableCollStats bool `yaml:"enable_coll_stats,omitempty"` | ||
TLSBasicAuthConfigPath string `yaml:"tls_basic_auth_config_path,omitempty"` | ||
} | ||
|
||
// UnmarshalYAML implements yaml.Unmarshaler for Config | ||
|
@@ -69,6 +83,25 @@ func New(logger log.Logger, c *Config) (integrations.Integration, error) { | |
} | ||
} | ||
|
||
if c.EnableDBStats || c.EnableDiagnosticData || | ||
c.EnableReplicasetStatus || c.EnableCollStats || | ||
c.EnableTopMetrics || c.EnableIndexStats { | ||
collectAll = false | ||
} | ||
|
||
if collectAll { | ||
c.EnableDBStats = true | ||
c.EnableDiagnosticData = true | ||
c.EnableReplicasetStatus = true | ||
c.EnableTopMetrics = true | ||
c.EnableIndexStats = true | ||
c.EnableCollStats = true | ||
} | ||
|
||
if c.EnableIndexStats || c.EnableCollStats { | ||
c.DiscoveringMode = true | ||
} | ||
|
||
exp := exporter.New(&exporter.Opts{ | ||
URI: string(c.URI), | ||
Logger: logrusLogger, | ||
|
@@ -78,10 +111,18 @@ func New(logger log.Logger, c *Config) (integrations.Integration, error) { | |
// names from mongodb_exporter <v0.20.0. Many existing dashboards rely on | ||
// the old names, so we hard-code it to true now. We may wish to make this | ||
// configurable in the future. | ||
CompatibleMode: true, | ||
CollectAll: true, | ||
DirectConnect: c.DirectConnect, | ||
DiscoveringMode: c.DiscoveringMode, | ||
CompatibleMode: true, | ||
DirectConnect: c.DirectConnect, | ||
DiscoveringMode: c.DiscoveringMode, | ||
|
||
CollectAll: collectAll, | ||
EnableDBStats: c.EnableDBStats, | ||
EnableDiagnosticData: c.EnableDiagnosticData, | ||
EnableReplicasetStatus: c.EnableReplicasetStatus, | ||
EnableTopMetrics: c.EnableTopMetrics, | ||
EnableIndexStats: c.EnableIndexStats, | ||
EnableCollStats: c.EnableCollStats, | ||
|
||
TLSConfigPath: c.TLSBasicAuthConfigPath, | ||
}) | ||
|
||
|
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.
This changelog entry is placed in a old release section. Please, move it to unreleased section.