Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## v3 #5134 +/- ##
==========================================
+ Coverage 47.15% 48.66% +1.51%
==========================================
Files 410 411 +1
Lines 41969 42093 +124
==========================================
+ Hits 19790 20485 +695
+ Misses 20234 19555 -679
- Partials 1945 2053 +108
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
|
||
| // DSN returns a DSN string for accessing a given Service with this Agent (and an implicit driver). | ||
| func (s *Agent) DSN(service *Service, dsnParams DSNParams, tdp *DelimiterPair, pmmAgentVersion *version.Parsed) string { //nolint:cyclop,maintidx | ||
| func (a *Agent) DSN(service *Service, dsnParams DSNParams, tdp *DelimiterPair, pmmAgentVersion *version.Parsed) string { //nolint:cyclop,maintidx |
There was a problem hiding this comment.
Not related, but we discussed this on Go BE.
managed/models/agent_model.go
Outdated
| MongoDBExporterType, | ||
| PostgresExporterType, | ||
| ProxySQLExporterType, | ||
| RDSExporterType, |
There was a problem hiding this comment.
QQ: Doesn't RDS Exporter have a higher default timeout?
There was a problem hiding this comment.
Right, it has usually around 90% of scrape interval. Will check it.
There was a problem hiding this comment.
Right now only scrape interval and scrape timeout from configs is used. Timeout in DSN is removed and not used like in v3.
managed/models/agent_model_test.go
Outdated
| } | ||
|
|
||
| expected := "redis://username:s3cur3%20p%40$$w0r4.@1.2.3.4:12345" | ||
| expected := "redis://username:s3cur3%20p%40$$w0r4.@1.2.3.4:12345?dial_timeout=1s" |
There was a problem hiding this comment.
From what I see in the documentation, Redix exporter does not support such a parameter in their connection string. They seem to use the --connection-timeout parameter or an env variable.
Let's double check to be sure.
There was a problem hiding this comment.
True, no DSN param, but flag. Should be correct right now. Also test file for this is added. Thank you.
| return err | ||
| } | ||
|
|
||
| exporterOptions := models.ExporterOptions{ |
There was a problem hiding this comment.
Is it justified to have it as a separate variable vs inline as before?
There was a problem hiding this comment.
Changed to original format.
| // (node / external / RDS scrape of exporter HTTP). By Prometheus rule, scrape_timeout | ||
| // cannot exceed scrape_interval, so PMM caps it at 0.9 of scrape interval: | ||
| // https://prometheus.io/docs/prometheus/latest/configuration/configuration/ | ||
| func applyExporterScrapeTimeout(cfg *config.ScrapeConfig, agent *models.Agent) { |
There was a problem hiding this comment.
Method names in Go don't need to be that long, I guess exporterScrapeTimeout is good enough, similar to the one right below :)
| ExtraDsnParams: agent.MySQLOptions.ExtraDSNParams, | ||
| }, nil | ||
| } | ||
| if agent.ExporterOptions.Timeout != 0 { |
There was a problem hiding this comment.
I'm not sure, but I'm wondering if this if statement is not redundant.
There was a problem hiding this comment.
agent.ExporterOptions.Timeout is not pointer so check could be skipped, but exporter.Timeout is so without this it will set 0 even for non set timeouts. With check it if more visible if it was really set or not.
| CustomLabels map[string]string `mapsep:"," help:"Custom user-assigned labels"` | ||
| PushMetrics bool `help:"Enables push metrics model flow, it will be sent to the server by an agent"` | ||
| TLSSkipVerify bool `help:"Skip TLS certificate verification"` | ||
| Timeout string `help:"Connection timeout to use for exporter (e.g. 1s, 500ms)"` |
There was a problem hiding this comment.
| Timeout string `help:"Connection timeout to use for exporter (e.g. 1s, 500ms)"` | |
| Timeout *time.Duration `placeholder:"DURATION" help:"Connection timeout to use for exporter (e.g. 1s, 500ms)"` |
There was a problem hiding this comment.
here and at the rest of commands
| // Metrics resolution for this agent. | ||
| common.MetricsResolutions metrics_resolutions = 15; | ||
| // Connection timeout for exporter (if set). | ||
| google.protobuf.Duration timeout = 16; |
There was a problem hiding this comment.
there may be many timeouts: connection, fetch metrics, etc.
it is better to explicitly define that timeout is ment here like connection_timeout
| // Real-Time Analytics options. | ||
| inventory.v1.RTAOptions rta_options = 42; | ||
| // Connection timeout for exporter (if set). | ||
| google.protobuf.Duration timeout = 43; |
There was a problem hiding this comment.
the same here and in the rest of the*.proto files
| // Maximum number of exporter connections to PostgreSQL instance. | ||
| int32 max_postgresql_exporter_connections = 33; | ||
| // Connection timeout for exporter (if set). | ||
| google.protobuf.Duration timeout = 35; |
There was a problem hiding this comment.
| google.protobuf.Duration timeout = 35; | |
| google.protobuf.Duration timeout = 34; |
| switch a.AgentType { | ||
| case NodeExporterType, | ||
| MySQLdExporterType, | ||
| MongoDBExporterType, | ||
| PostgresExporterType, | ||
| ProxySQLExporterType, | ||
| AzureDatabaseExporterType, | ||
| ExternalExporterType, | ||
| ValkeyExporterType: | ||
| return 1 * time.Second | ||
| default: | ||
| } |
There was a problem hiding this comment.
should it be there if effectiveDialTimeoutFallbackSec is used as fallback?
| QANMongoDBMongologAgentType, | ||
| MongoDBExporterType, | ||
| RTAMongoDBAgentType) | ||
| MongoDBExporterType) |
There was a problem hiding this comment.
- Why RTA and MongoLog agents types have been removed?
- Why MongoProfiler type is kept?
There was a problem hiding this comment.
I think I resolved conflict during merging main wrong. I will fix it, thanks
PMM-12832
FB: Percona-Lab/pmm-submodules#4275