Tibi holmes - #5118
Conversation
… collector - config: add Paths.OtelCollector, default tools/otelcol-contrib - supervisor: processParams case for AGENT_TYPE_OTEL_COLLECTOR with server_otlp_url, server_auth_b64, server_insecure from server config
- add_otel.go: AddOtelCommand with PMMAgentID (autodetect), log-file-paths, custom-labels; calls Inventory AddAgent with otel_collector body - add.go: register Otel subcommand under add Requires 'make gen' for AddAgentParamsBodyOtelCollector / AddAgentOKBodyOtelCollector.
- datasources.yml: add ClickHouse-OTEL datasource (defaultDatabase: otel) - dev/otel/README.md: architecture, enabling, config, extensibility - dev/otel/TESTING.md: prerequisites, add otel, OTLP, ClickHouse, Grafana
- Added OtelCollector type to the agent inventory, including its properties and validation methods. - Updated API responses and requests to include OtelCollector in ListAgentsResponse, GetAgentResponse, AddAgentRequest, and AddAgentResponse. - Enhanced validation for OtelCollector in AddAgent and GetAgent requests. - Updated JSON schemas to reflect the new OtelCollector structure and properties. This commit requires running 'make gen' to regenerate the necessary API types.
- Updated the configuration tests in config_test.go to include the OtelCollector path. - Ensured consistency across multiple test cases by adding the OtelCollector property to the relevant structures.
- Updated the supervisor process to use strconv.FormatBool for server_insecure parameter. - Added a new OTEL schema management package to handle ClickHouse database and table creation. - Introduced a new configuration file for the OTEL collector in supervisord. - Enhanced the server package to include the new OTEL schema management. - Made minor formatting adjustments across various files for consistency.
- Updated conditions in otelcollector.go and agents.go to use 'len() != 0' for checking log file paths instead of 'len() > 0' for improved clarity and consistency.
…AddOtelCollector method - Set OtelCollectorEnabledDefault to true in settings.go. - Added nil parameter check in AddOtelCollector method in agents.go. - Updated grpc agents_server to validate parameters for AddOtelCollector request.
- Updated EnsureOtelSchema function to accept a ClickHouse DSN instead of separate address, user, and password parameters. - Introduced default values for ClickHouse user and password. - Modified EnsureOtelSchemaFromEnv to construct the DSN using environment variables for improved flexibility and security. - Updated documentation comments for clarity on DSN format and usage.
- Included the otelcol-contrib binary in the PMM tools directory for both deb and rpm packages. - Updated installation scripts to ensure the binary is downloaded and installed correctly based on the architecture. - Modified testing documentation to reflect the inclusion of otelcol-contrib in the PMM client and server packages.
- Updated the script to dynamically find the otelcol-contrib binary after extraction, enhancing robustness. - Added error handling to ensure the binary is present before proceeding with the copy operation, improving reliability during the build process.
- Introduced a new configuration file for the OTEL collector in `otelconfig.go`, defining the YAML structure for OTLP and filelog receivers, processors, and ClickHouse exporter. - Added comprehensive unit tests in `otelconfig_test.go` to validate the configuration generation and ensure correct YAML structure and content. - Updated the `supervisord.go` to handle writing the OTEL collector configuration to the appropriate directory, enhancing the service's configuration management.
- Created the '/srv/otelcol' directory for otel-collector configuration with appropriate ownership and permissions. - Updated entrypoint script to include 'otelcol' in the directory creation process. - Changed configuration paths in supervisord.go and related files to point to the new '/srv/otelcol' directory, ensuring consistency across the application.
- Introduced a new LogSource message to bind log file paths to parser presets, allowing for more flexible log collection configurations. - Updated AddOtelCollectorParams to include log_sources, deprecating log_file_paths for improved clarity and functionality. - Implemented validation for LogSource to ensure proper path and preset handling. - Enhanced OTEL collector configuration to support per-path presets, improving log parsing capabilities. - Updated related documentation and README to reflect changes in log source management and usage.
- Changed the OTEL exporter name from 'otlp' to 'otlp_http' for clarity in configuration. - Adjusted timestamp layout in the database schema to remove unnecessary precision. - Added a new route for OTLP ingest in the Grafana auth server, allowing viewer access for log and trace pushes.
- Added functionality to retrieve and include resource attributes (agent_id, node_id, service_name) in the OTEL collector configuration for improved log correlation with ClickHouse. - Updated baseOtelConfigYaml to conditionally include a resource processor when resource attributes are present, ensuring logs match VictoriaMetrics labels. - Introduced a new utility function, quoteYAMLAttrValue, for safely quoting YAML attribute values. - Modified the OTEL configuration in supervisord to set the pmm_source attribute for better log context.
- Updated the AddOtelCommand structure to include detailed help descriptions for log sources and parser presets, listing available presets for clarity. - Added new log parser presets for nginx access, nginx error, grafana, pmm-managed, pmm-agent, and postgres to the database schema, improving log parsing capabilities. - Implemented default log sources for the server's OTEL collector, ensuring automatic configuration of log paths and presets. - Refined the OTEL collector configuration to focus on OTLP receiver only, with all log collection handled by pmm-agent using database-driven presets. - Updated related tests to reflect changes in the configuration and ensure proper functionality.
- Updated the UpdateConfiguration method in supervisord to accept an optional otelConfigContent parameter, allowing for dynamic OTEL collector configuration. - Implemented logic in the server to build and pass the OTEL configuration content based on settings, improving flexibility in log collection. - Added new log parser presets for ClickHouse and OTEL collector logs, enhancing log parsing capabilities. - Introduced tests for the new OTEL configuration generation and validation, ensuring proper functionality and integration. - Refactored related code to support the new configuration structure and improve overall maintainability.
- Introduced a new pmm-managed binary file to the project. - Refactored the setup function to utilize server.UpdateConfigurations for OTEL configuration, ensuring proper handling of log sources and avoiding duplicate YAML keys. - Improved logging for configuration updates and error handling during the setup process.
- Settings: Add Adre struct (Enabled, URL), IsAdreEnabled(), GetAdreURL() - ChangeSettingsParams: Add EnableAdre, AdreURL for updates - New package managed/services/adre: HolmesGPT HTTP client - Models(), Chat(), ChatStream(), Investigate(), InvestigateStream() - Uses HolmesGPT /api/chat, /api/stream/chat, /api/investigate, etc. Made-with: Cursor
- Introduced new HTTP handlers for ADRE settings, models, chat, alerts, and investigation endpoints in the pmm-managed service. - Updated the Grafana auth server to include permissions for ADRE routes. - Added navigation support for ADRE in the UI, including a dedicated page and chat widget. - Enhanced the main application layout to incorporate the ADRE chat widget. Made-with: Cursor
# Conflicts: # .golangci.yml # managed/utils/env/env.go # managed/utils/envvars/parser_test.go
percona-robot
left a comment
There was a problem hiding this comment.
Remaining comments which cannot be posted as a review comment to avoid GitHub Rate Limit
golangci
🚫 [golangci] reported by reviewdog 🐶
avoid inline error handling using if err := ...; err != nil; use plain assignment err := ... (noinlineerr)
🚫 [golangci] reported by reviewdog 🐶
directive //nolint:contextcheck // background run uses a fresh detached context is unused for linter "contextcheck" (nolintlint)
pmm/managed/services/investigations/chat.go
Line 248 in 669fd7f
🚫 [golangci] reported by reviewdog 🐶
directive //nolint:gocognit is unused for linter "gocognit" (nolintlint)
pmm/managed/services/slackbot/run.go
Line 251 in 669fd7f
🚫 [golangci] reported by reviewdog 🐶
named return "serviceNow" with type "bool" found (nonamedreturns)
pmm/managed/models/adre_deployment_helpers.go
Line 371 in 669fd7f
🚫 [golangci] reported by reviewdog 🐶
redefines-builtin-id: redefinition of the built-in function cap (revive)
|
|
||
| // encryptSecrets encrypts each value for at-rest storage, aborting on the first error so a secret is | ||
| // never persisted in plaintext. The returned slice is in the same order as the arguments. | ||
| // encryption.Encrypt is a no-op on the empty string. |
There was a problem hiding this comment.
🚫 [golangci] reported by reviewdog 🐶
Sentence should start with a capital letter (godot)
|
|
||
| // decryptField decrypts an at-rest secret. On error (e.g. a legacy plaintext value) it returns the | ||
| // original value so reads degrade gracefully, logging only the field name — never the encryption.Decrypt | ||
| // error, which embeds the raw input. encryption.Decrypt is a no-op on the empty string. |
There was a problem hiding this comment.
🚫 [golangci] reported by reviewdog 🐶
Sentence should start with a capital letter (godot)
| const ( | ||
| maxThreadMessagesRAM = 50 | ||
| maxConcurrentAdreChatSlots = 3 | ||
| // maxSlackUserTextBytes caps the human chat text sent to Holmes (bounds token cost + injection |
There was a problem hiding this comment.
🚫 [golangci] reported by reviewdog 🐶
Sentence should start with a capital letter (godot)
| ) | ||
|
|
||
| const ( | ||
| // slackRatePerMinute / slackRateBurst bound human chat turns per Slack user. |
There was a problem hiding this comment.
🚫 [golangci] reported by reviewdog 🐶
Sentence should start with a capital letter (godot)
| // slackRatePerMinute / slackRateBurst bound human chat turns per Slack user. | ||
| slackRatePerMinute = 5 | ||
| slackRateBurst = 3 | ||
| // denyCooldownWindow throttles the "ask an admin" / "too fast" replies per (channel,user). |
There was a problem hiding this comment.
🚫 [golangci] reported by reviewdog 🐶
Sentence should start with a capital letter (godot)
| t.Error("negative hourly cap should fail") | ||
| } | ||
| matchers := []string{"no-equals-sign"} | ||
| if err := validateAdreSlackListParams(&ChangeSettingsParams{AutoInvestigateLabelMatchers: &matchers}); err == nil { |
There was a problem hiding this comment.
🚫 [golangci] reported by reviewdog 🐶
avoid inline error handling using if err := ...; err != nil; use plain assignment err := ... (noinlineerr)
| State string `json:"state"` | ||
| } `json:"status"` | ||
| } | ||
| if err := json.Unmarshal(raw, &items); err != nil { |
There was a problem hiding this comment.
🚫 [golangci] reported by reviewdog 🐶
avoid inline error handling using if err := ...; err != nil; use plain assignment err := ... (noinlineerr)
| Annotations map[string]string `json:"annotations"` | ||
| } `json:"alerts"` | ||
| } | ||
| if err := json.Unmarshal(raw, &payload); err != nil { |
There was a problem hiding this comment.
🚫 [golangci] reported by reviewdog 🐶
avoid inline error handling using if err := ...; err != nil; use plain assignment err := ... (noinlineerr)
| Role: "user", | ||
| Content: "Generate the full investigation report.", | ||
| } | ||
| if mErr := models.CreateInvestigationMessage(h.db, userMsg); mErr != nil { |
There was a problem hiding this comment.
🚫 [golangci] reported by reviewdog 🐶
avoid inline error handling using if err := ...; err != nil; use plain assignment err := ... (noinlineerr)
| var alerts []struct { | ||
| Fingerprint string `json:"fingerprint"` | ||
| } | ||
| if err := json.Unmarshal(raw, &alerts); err != nil { |
There was a problem hiding this comment.
🚫 [golangci] reported by reviewdog 🐶
avoid inline error handling using if err := ...; err != nil; use plain assignment err := ... (noinlineerr)
| w.WriteHeader(http.StatusAccepted) | ||
| _ = json.NewEncoder(w).Encode(map[string]string{"status": "running"}) //nolint:errchkjson // response already committed | ||
|
|
||
| go h.runInvestigationBackground(id, nil, settings) //nolint:contextcheck // background run uses a fresh detached context |
There was a problem hiding this comment.
🚫 [golangci] reported by reviewdog 🐶
directive //nolint:contextcheck // background run uses a fresh detached context is unused for linter "contextcheck" (nolintlint)
| // AdreSecretsConfigured reports whether the ServiceNow tokens and the Slack tokens are set, checking | ||
| // the encrypted columns for non-emptiness without decrypting them (Encrypt maps "" to "" and non-empty | ||
| // to non-empty ciphertext, so emptiness is preserved). Used for the masked settings API. | ||
| func AdreSecretsConfigured(q reform.DBTX) (serviceNow, slack bool, err error) { |
There was a problem hiding this comment.
🚫 [golangci] reported by reviewdog 🐶
named return "serviceNow" with type "bool" found (nonamedreturns)
… based on ADRE settings Signed-off-by: theTibi <tkorocz@gmail.com>
…auto-investigate settings Signed-off-by: theTibi <tkorocz@gmail.com>
# Conflicts: # go.mod
…plify parameters Signed-off-by: theTibi <tkorocz@gmail.com>
- Introduced SlackRef struct to link alerts to their originating Slack messages, enabling in-thread replies for investigation notices. - Updated Alert struct to include SlackRef for alerts scraped from Slack messages. - Modified buildAlertInvestigation to carry Slack thread information for posting investigation reports. - Implemented report posting in Slack threads upon investigation completion. - Enhanced parsing of remediation steps to skip Markdown code-fence markers. - Added tests for parsing Slack alerts and remediation steps. - Updated API to support filtering investigations by trigger type (auto/manual). - Improved UI components for displaying investigation lists and remediation steps. - Added configuration options for Slack alert bot IDs to restrict alert sources. Signed-off-by: theTibi <tkorocz@gmail.com>
# Conflicts: # .gitignore
| // don't have to separately re-run deployment provisioning after toggling it on. Idempotent and | ||
| // best-effort: the auto-investigate reconciliation poll works even if this fails. | ||
| if settings != nil && settings.Adre.SlackAutoInvestigate { | ||
| if err := h.provisioner().EnsureAlertWebhook(incomingAuthContext(r)); err != nil { |
There was a problem hiding this comment.
🚫 [golangci] reported by reviewdog 🐶
Non-inherited new context, use function like context.WithXXX or r.Context instead (contextcheck)
| if v := a.Labels["cluster"]; v != "" { | ||
| cfg["cluster_name"] = v | ||
| } | ||
| snapshot, _ := json.Marshal([]alertSnapshotEntry{{ |
There was a problem hiding this comment.
🚫 [golangci] reported by reviewdog 🐶
Error return value of encoding/json.Marshal is not checked (errchkjson)
| var allowedOrder = map[string]bool{"asc": true, "desc": true} | ||
|
|
||
| // ListInvestigations returns investigations with optional status and trigger filters and configurable | ||
| // sort. StatusFilter empty means all statuses. triggerFilter is "auto" (created by the auto-investigate |
There was a problem hiding this comment.
🚫 [golangci] reported by reviewdog 🐶
Sentence should start with a capital letter (godot)
| // EnsureAlertWebhook idempotently provisions the auto-investigate webhook secret plus the Grafana | ||
| // contact point + catch-all route that deliver firing alerts to PMM's authenticated alert webhook. | ||
| // Best-effort: callers log failures and continue (auto-investigate still runs via the reconciliation | ||
| // poll). ctx must carry the admin auth headers — the Grafana provisioning API requires them. |
There was a problem hiding this comment.
🚫 [golangci] reported by reviewdog 🐶
Sentence should start with a capital letter (godot)
| } | ||
|
|
||
| // slackThreadRef returns the (channel, thread_ts) an investigation was scraped from, or empties when it | ||
| // did not originate from a Slack message (webhook/poll path). buildAlertInvestigation stores these in |
There was a problem hiding this comment.
🚫 [golangci] reported by reviewdog 🐶
Sentence should start with a capital letter (godot)
| msg := fmt.Sprintf("🔎 Auto-investigation started for *%s*.%s", inv.Title, link) | ||
|
|
||
| if threadChannel != "" { | ||
| if _, _, err := api.PostMessageContext(ctx, threadChannel, slack.MsgOptionText(msg, false), slack.MsgOptionTS(threadTS)); err != nil { |
There was a problem hiding this comment.
🚫 [golangci] reported by reviewdog 🐶
avoid inline error handling using if err := ...; err != nil; use plain assignment err := ... (noinlineerr)
| return | ||
| } | ||
| for _, ch := range channels { | ||
| if _, _, err := api.PostMessageContext(ctx, ch, slack.MsgOptionText(msg, false)); err != nil { |
There was a problem hiding this comment.
🚫 [golangci] reported by reviewdog 🐶
avoid inline error handling using if err := ...; err != nil; use plain assignment err := ... (noinlineerr)
| // gateAndHandle authorizes and rate-limits a human Slack interaction before running a turn. Denied | ||
| // interactions get one cooled-down "ask an admin" reply; throttled ones get one cooled-down "too | ||
| // fast" reply. Both keep the bot from being weaponized into a reply-spam amplifier. | ||
| func gateAndHandle( //nolint:gocognit |
There was a problem hiding this comment.
🚫 [golangci] reported by reviewdog 🐶
directive //nolint:gocognit is unused for linter "gocognit" (nolintlint)
| // rather than in-memory reservations (so duplicates/failures never burn the budget). This is a | ||
| // soft cap — concurrent claimers can each read the count before the other commits, so it may be | ||
| // exceeded by up to (concurrent processors − 1) within a window. | ||
| if cap := settings.Adre.AutoInvestigateHourlyCap; cap > 0 { |
There was a problem hiding this comment.
🚫 [golangci] reported by reviewdog 🐶
redefines-builtin-id: redefinition of the built-in function cap (revive)
| // resolution (or the summary as a fallback) + a link to the full report in the UI. | ||
| func buildReportSummary(inv *models.Investigation, link string) string { | ||
| var b strings.Builder | ||
| b.WriteString(fmt.Sprintf("✅ Investigation complete: *%s*\n", inv.Title)) |
There was a problem hiding this comment.
🚫 [golangci] reported by reviewdog 🐶
QF1012: Use fmt.Fprintf(...) instead of WriteString(fmt.Sprintf(...)) (staticcheck)
…rements - Added `EnvRestartRequired` flag to indicate when a container restart is needed for .env changes (e.g., PMM URL, secrets). - Updated provisioning logic to set restart flags appropriately when changes occur. - Implemented `PostDeploymentRestarted` endpoint to acknowledge container recreation and clear restart flags. - Enhanced API responses to include reload outcomes, indicating whether changes were applied live or require a restart. - Updated UI components to reflect new restart requirements and provide user feedback on the status of changes. - Added tests to verify the behavior of hot-reload and restart acknowledgment functionalities. Signed-off-by: theTibi <tkorocz@gmail.com>
…ation for enhanced API interactions Signed-off-by: theTibi <tkorocz@gmail.com>
Signed-off-by: theTibi <tkorocz@gmail.com>
- Removed EnsureAlertWebhookSecret function and its associated secret generation logic. - Deleted alert_webhook.go as it is no longer needed for processing Grafana alert webhooks. - Updated Provisioner to eliminate auto-investigate webhook provisioning, relying on reconciliation poll instead. - Adjusted GrafanaAuth and Handlers interfaces to remove references to alert webhook contact points. - Modified handlers to reflect changes in alert processing, focusing on Slack alert scraping. - Enhanced SlackNotifier to handle investigation reports and notifications more effectively. - Added tests for new Slack message handling logic and ensured proper mention stripping to prevent loops. Signed-off-by: theTibi <tkorocz@gmail.com>
…on poll references Signed-off-by: theTibi <tkorocz@gmail.com>
…GPT (ADRE) Signed-off-by: theTibi <tkorocz@gmail.com>
PMM-0
Link to the Feature Build: SUBMODULES-0
If this PR adds, removes or alters one or more API endpoints, please review and add or update the relevant API documentation as well:
If this PR is related to some other PRs in this or other repositories, please provide links to those PRs: