fix logs #537
fix logs #537
Conversation
dosubot
bot
added
the
size:M
WalkthroughThis pull request introduces a comprehensive HelmRelease configuration for monitoring agents in a Kubernetes environment. The configuration defines a structured approach to deploying monitoring tools like Changes
Sequence DiagramsequenceDiagram
participant Cluster as Kubernetes Cluster
participant HelmRelease as Monitoring Agents HelmRelease
participant VmAgent as VmAgent
participant FluentBit as Fluent-Bit
Cluster->>HelmRelease: Check addon enabled
alt Addon Enabled
HelmRelease->>VmAgent: Deploy with external labels
HelmRelease->>FluentBit: Deploy with log processing config
FluentBit->>Cluster: Collect and forward logs
end
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
packages/system/monitoring-agents/values.yaml (2)
309-313: Enhance readiness probe configurationThe current readiness probe configuration is minimal. Consider adding the following parameters for more robust health checking:
- Use
/api/v1/healthinstead of root path- Add appropriate timeouts and thresholds
fluent-bit: readinessProbe: httpGet: - path: / + path: /api/v1/health + initialDelaySeconds: 10 + periodSeconds: 30 + timeoutSeconds: 5 + failureThreshold: 3
313-325: Add SELinux configuration for volume mountsThe volume mounts look correct, but consider adding SELinux configuration to prevent permission issues on clusters with SELinux enabled.
daemonSetVolumes: - name: varlog hostPath: path: /var/log + type: Directory + securityContext: + seLinuxOptions: + type: spc_t - name: varlibdockercontainers hostPath: path: /var/lib/docker/containers + type: Directory + securityContext: + seLinuxOptions: + type: spc_t
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/apps/kubernetes/templates/helmreleases/monitoring-agents.yaml(0 hunks)packages/system/monitoring-agents/values.yaml(1 hunks)
💤 Files with no reviewable changes (1)
- packages/apps/kubernetes/templates/helmreleases/monitoring-agents.yaml
🔇 Additional comments (1)
packages/system/monitoring-agents/values.yaml (1)
308-308: Verify service endpoints configuration
Please verify that the service endpoints are correctly configured and accessible:
- vmagent writes to:
vminsert-shortterm.tenant-root.svc:8480 - fluent-bit writes to:
vlogs-generic.tenant-root.svc:9428
Also applies to: 331-332
✅ Verification successful
Service endpoints are correctly templated and consistent
The service endpoints are correctly configured in both the values file and the Helm template:
- The endpoints use consistent templating where
tenant-rootis the default value invalues.yaml - The Helm template in
monitoring-agents.yamlproperly parameterizes the tenant using{{ $targetTenant }} - Port numbers (8480 and 9428) are consistent across configurations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the existence of referenced services in the tenant-root namespace
# Check if the services exist in the tenant-root namespace
kubectl get svc vminsert-shortterm vlogs-generic -n tenant-root -o name
# Check if the ports are defined in the service specs
kubectl get svc vminsert-shortterm vlogs-generic -n tenant-root -o jsonpath='{range .items[*]}{.metadata.name}{" ports: "}{.spec.ports[*].port}{"\n"}{end}'
Length of output: 427
Script:
#!/bin/bash
# Let's verify the service endpoints configuration in the values.yaml file
rg -A 2 "vminsert-shortterm.tenant-root.svc:8480|vlogs-generic.tenant-root.svc:9428" packages/system/monitoring-agents/values.yaml
# Also search for any service endpoint configurations in the entire codebase
rg -A 2 "vminsert-shortterm|vlogs-generic" --type yaml
Length of output: 1402
| outputs: | | ||
| [OUTPUT] | ||
| Name http | ||
| Match kube.* | ||
| Host vlogs-generic.tenant-root.svc | ||
| port 9428 | ||
| compress gzip | ||
| uri /insert/jsonline?_stream_fields=stream,kubernetes_pod_name,kubernetes_container_name,kubernetes_namespace_name&_msg_field=log&_time_field=date | ||
| format json_lines | ||
| json_date_format iso8601 | ||
| header AccountID 0 | ||
| header ProjectID 0 | ||
| filters: | |
There was a problem hiding this comment.
Enhance output configuration for reliability and security
The current output configuration lacks important reliability and security features:
- Missing retry mechanism for network failures
- No TLS configuration for secure communication
- Hard-coded AccountID/ProjectID values
outputs: |
[OUTPUT]
Name http
Match kube.*
Host vlogs-generic.tenant-root.svc
port 9428
compress gzip
uri /insert/jsonline?_stream_fields=stream,kubernetes_pod_name,kubernetes_container_name,kubernetes_namespace_name&_msg_field=log&_time_field=date
format json_lines
json_date_format iso8601
+ tls On
+ tls.verify On
+ retry_limit 5
+ net.keepalive On
+ net.keepalive_idle_timeout 30
header AccountID 0
header ProjectID 0📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| outputs: | | |
| [OUTPUT] | |
| Name http | |
| Match kube.* | |
| Host vlogs-generic.tenant-root.svc | |
| port 9428 | |
| compress gzip | |
| uri /insert/jsonline?_stream_fields=stream,kubernetes_pod_name,kubernetes_container_name,kubernetes_namespace_name&_msg_field=log&_time_field=date | |
| format json_lines | |
| json_date_format iso8601 | |
| header AccountID 0 | |
| header ProjectID 0 | |
| filters: | | |
| outputs: | | |
| [OUTPUT] | |
| Name http | |
| Match kube.* | |
| Host vlogs-generic.tenant-root.svc | |
| port 9428 | |
| compress gzip | |
| uri /insert/jsonline?_stream_fields=stream,kubernetes_pod_name,kubernetes_container_name,kubernetes_namespace_name&_msg_field=log&_time_field=date | |
| format json_lines | |
| json_date_format iso8601 | |
| tls On | |
| tls.verify On | |
| retry_limit 5 | |
| net.keepalive On | |
| net.keepalive_idle_timeout 30 | |
| header AccountID 0 | |
| header ProjectID 0 | |
| filters: | |
| filters: | | ||
| [FILTER] | ||
| Name kubernetes | ||
| Match kube.* | ||
| Merge_Log On | ||
| Keep_Log On | ||
| K8S-Logging.Parser On | ||
| K8S-Logging.Exclude On | ||
| [FILTER] | ||
| Name nest | ||
| Match * | ||
| Wildcard pod_name | ||
| Operation lift | ||
| Nested_under kubernetes | ||
| Add_prefix kubernetes_ | ||
| [FILTER] | ||
| Name modify | ||
| Match * | ||
| Add tenant tenant-root | ||
| [FILTER] | ||
| Name modify | ||
| Match * | ||
| Add cluster root-cluster |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Parameterize tenant and cluster names in filters
The tenant and cluster names are currently hard-coded. These should be parameterized to support different environments and multi-tenant deployments.
filters: |
[FILTER]
Name modify
Match *
- Add tenant tenant-root
+ Add tenant ${TENANT_NAME}
[FILTER]
Name modify
Match *
- Add cluster root-cluster
+ Add cluster ${CLUSTER_NAME}Additionally, consider adding a filter to handle multiline logs (e.g., stack traces) before the Kubernetes filter:
filters: |
+ [FILTER]
+ Name multiline
+ Match *
+ multiline.key_content log
+ multiline.parser java
[FILTER]
Name kubernetesCommittable suggestion skipped: line range outside the PR's diff.
Summary by CodeRabbit
New Features
fluent-bitwith configurations for readiness probes, volumes, and log processing.Bug Fixes