Send agent telemetry #595
Send agent telemetry #595
Conversation
|
Warning Rate limit exceeded@nityanandagohain has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 9 minutes and 24 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughThis pull request introduces enhancements to the OpenTelemetry telemetry configuration in the Kubernetes infrastructure chart. The changes include adding a new Changes
Sequence DiagramsequenceDiagram
participant Values as values.yaml
participant Config as _config.tpl
participant Agent as OTel Agent
participant Deployment as OTel Deployment
Values->>Config: Configure self-telemetry settings
Config->>Agent: Apply telemetry configuration
Config->>Deployment: Apply telemetry configuration
Agent-->>Values: Validate log collection
Deployment-->>Values: Validate metrics and traces
Possibly related PRs
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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.gitignore(1 hunks)charts/k8s-infra/templates/_config.tpl(7 hunks)charts/k8s-infra/values.yaml(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- .gitignore
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test-chart
🔇 Additional comments (10)
charts/k8s-infra/templates/_config.tpl (8)
22-30: Appropriate inclusion of ownTelemetry configurationsThe conditional blocks correctly check for
.Values.presets.ownTelemetrysettings and include the appropriate configurations for traces, metrics, and logs. This enhances the flexibility and customization of the telemetry setup.
52-54: Ensure correct integration of ownTelemetry logs exporterIncluding the ownTelemetry OTLP exporter configuration for logs is essential. Verify that this exporter is properly configured and does not conflict with other exporters in the pipeline.
77-82: Consistent application of ownTelemetry configurations in OtelDeploymentThe added conditional blocks ensure that ownTelemetry configurations for traces and metrics are applied to
otelDeployment, maintaining consistency withotelAgent.
107-124: Well-structured helper functions for ownTelemetry configurationsThe introduction of helper functions
applyOwnTracesConfig,applyOwnMetricsConfig, andapplyOwnLogsConfigenhances code modularity and reusability. This structure promotes maintainability and simplifies future updates.
170-174: Introduction ofapplyOtlpExporterOwnTelemetryConfigmethodThis method allows for custom exporter configurations for ownTelemetry, providing flexibility in specifying endpoints and TLS settings.
178-187: Proper configuration ofotlphttp/own_telemetryexporterThe exporter is correctly configured to use the endpoint and TLS settings from ownTelemetry presets, ensuring secure and customizable data transmission.
495-498: Addingresourcedetectionprocessor tologs/own_logspipelineIncluding the
resourcedetectionprocessor enhances the metadata attached to logs in thelogs/own_logspipeline, which can be beneficial for observability and analysis.
516-519: Ensuringresourcedetectionprocessor is applied in OtelDeploymentApplying the
resourcedetectionprocessor to thelogs/own_logspipeline inotelDeploymentmaintains consistency withotelAgentconfigurations and ensures that resource attributes are properly detected.charts/k8s-infra/values.yaml (2)
93-105: Addition ofownTelemetryconfiguration enhances flexibilityThe introduction of the
ownTelemetrysection underpresetsallows for customizable telemetry configurations for traces, metrics, and logs, improving the observability capabilities of the system.
108-108: Verify the change ofstartAtparameter for logs collectionThe
startAtparameter has been changed frombeginningtoend. This means that log collection will start from new logs rather than existing logs in the files. Ensure that this change aligns with the intended behavior and does not result in missing important historical logs.
|
will do few more checks before opening for review |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
charts/k8s-infra/values.yaml (1)
93-105:⚠️ Potential issueSecurity: Review default values for insecure settings
The default configuration has
insecure: trueandinsecureSkipVerify: truewhich could expose the system to security risks. Consider:
- Setting these to
falseby default to enforce secure communication- Adding validation for the endpoint format when specified
- Adding documentation about security implications
Apply this diff to update the default values:
ownTelemetry: # OTLP HTTP endpoint to send own telemetry data to. endpoint: "" # Whether to use insecure mode. - insecure: true + insecure: false # Whether to skip verifying the certificate. - insecureSkipVerify: true + insecureSkipVerify: false
🧹 Nitpick comments (1)
charts/k8s-infra/templates/_config.tpl (1)
289-292: Consider renaming the filter for clarityWhile the filter logic is correct (dropping non-error logs), the name
filter/non_error_logscould be clearer. Consider renaming to better reflect its purpose of keeping only error logs.- filter/non_error_logs: + filter/keep_error_logs: logs: log_record: - 'not IsMatch(body, ".*error.*")'
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
charts/k8s-infra/templates/_config.tpl(7 hunks)charts/k8s-infra/values.yaml(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test-chart
🔇 Additional comments (4)
charts/k8s-infra/values.yaml (1)
108-108: LGTM: Log collection configuration is correctSetting
startAt: endensures that deployment logs are not collected, as these will already be sent by the agent. This aligns with the PR objectives.charts/k8s-infra/templates/_config.tpl (3)
22-33: LGTM: Well-structured telemetry configurationThe implementation follows good practices:
- Clear separation of telemetry types
- Conditional application of configurations
- Proper feature toggling
204-222: LGTM: Robust telemetry endpoint configurationThe implementation properly handles:
- Custom endpoint configuration with HTTPS support
- Fallback to environment variables
- API key authentication
649-652: LGTM: Proper resource detection for own logsThe changes correctly add resource detection processor to the own logs pipeline, ensuring consistent resource attribution across all telemetry data.
Also applies to: 673-676
…ing custom otelAgent config
Continuation of the above... Ideally, you can set additional environment variables with OTEL endpoint and credentials when Refer here: https://github.com/SigNoz/charts/blob/main/charts/k8s-infra/templates/_helpers.tpl#L404-L410 |
|
OpenTelemetry uses "own" in a number of places to refer to the agent's own operational metrics; this is the reason I used "own" in the other PR, which Nitya continued. @nityanandagohain Please change back because we want to be consistent with upstream as much as possible. "Collector instance by checking its own internal telemetry" |
|
I'm sorry, where are they using it apart from the opamp protocol spec? |
|
Two links I shared refer to internal telemetry as "own telemetry", and that's how it is commonly referred to in my collaboration experience with upstream. When there is a precedent set upstream, it's good to use the same independent of what we prefer individually. |
|
They're referring to collector's telemetry as "self" being much more widely used in every programming language/construct. The upstream collector also has a milestone called "Self Telemetry". https://github.com/open-telemetry/opentelemetry-collector/milestone/82. The choice can be between "internal" and "self". |
|
I will not drag this further because it is unproductive back and forth about the choice of name. My suggestion for using own_metrics/logs/traces stems from maintaining consistency with the existing agent management specification and implementation. Use whatever. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
charts/k8s-infra/tests/otel-deployment_self_telemetry_test.yaml (1)
75-75:⚠️ Potential issueUpdate endpoint to use HTTPS protocol.
The test configuration uses plain HTTP which doesn't align with the PR objectives for HTTPS endpoint configuration.
Apply this diff to update the endpoint configuration:
- endpoint: http://${env:OTEL_EXPORTER_OTLP_ENDPOINT} + endpoint: http{{ if not .Values.otelInsecure }}s{{ end }}://${env:OTEL_EXPORTER_OTLP_ENDPOINT}Also applies to: 92-92
🧹 Nitpick comments (1)
charts/k8s-infra/values.yaml (1)
94-95: Add HTTPS scheme to the endpoint comment.The comment should specify that HTTPS is the default scheme for the endpoint.
Apply this diff to update the comment:
- # OTLP HTTP endpoint to send own telemetry data to. + # OTLP HTTPS endpoint to send self telemetry data to (e.g., https://example.com:4318).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
charts/k8s-infra/templates/_config.tpl(7 hunks)charts/k8s-infra/templates/_helpers.tpl(2 hunks)charts/k8s-infra/templates/apikey-self-telemetry-secret.yaml(1 hunks)charts/k8s-infra/tests/otel-agent_self_telemetry_test.yaml(1 hunks)charts/k8s-infra/tests/otel-deployment_self_telemetry_test.yaml(1 hunks)charts/k8s-infra/values.yaml(1 hunks)
🧰 Additional context used
🪛 yamllint (1.35.1)
charts/k8s-infra/templates/apikey-self-telemetry-secret.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
charts/k8s-infra/tests/otel-agent_self_telemetry_test.yaml
[error] 135-135: no new line character at the end of file
(new-line-at-end-of-file)
charts/k8s-infra/tests/otel-deployment_self_telemetry_test.yaml
[error] 99-99: no new line character at the end of file
(new-line-at-end-of-file)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test-chart
🔇 Additional comments (12)
charts/k8s-infra/templates/_helpers.tpl (2)
411-417: LGTM! Environment variable configuration follows best practices.The implementation correctly handles the self telemetry API key using Kubernetes secrets and follows the established pattern.
454-474: LGTM! Helper functions are well-implemented.The helper functions for self telemetry API key secret management follow the established pattern and include proper validation.
charts/k8s-infra/templates/apikey-self-telemetry-secret.yaml (1)
1-14: LGTM! Secret template follows Kubernetes best practices.The implementation correctly:
- Uses Helm hooks for proper lifecycle management
- Conditionally creates secret only when needed
- Properly encodes the API key
🧰 Tools
🪛 yamllint (1.35.1)
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
charts/k8s-infra/tests/otel-deployment_self_telemetry_test.yaml (1)
39-64: LGTM! Test correctly verifies no log pipeline for deployment.The test configuration properly validates that deployment telemetry excludes log collection, aligning with the PR objectives.
charts/k8s-infra/tests/otel-agent_self_telemetry_test.yaml (1)
66-79: LGTM! Log configuration aligns with requirements.The implementation correctly:
- Filters for error logs using the filter processor
- Sets log reading from the end with
start_at: endcharts/k8s-infra/templates/_config.tpl (5)
22-33: LGTM: Well-structured conditional configuration applicationThe configuration application logic is well-organized, applying self telemetry configurations only when the respective features are enabled.
Also applies to: 80-88
649-651: LGTM: Resource detection properly applied to self logs pipelineThe resource detection processor is correctly applied to the self logs pipeline, ensuring proper resource attribution.
Also applies to: 672-674
122-144: 🛠️ Refactor suggestionMaintain consistent terminology: Use "self" instead of "own"
Based on the discussion in the PR comments about terminology consistency, we should use "self" consistently throughout the codebase.
Apply this diff to update the function names:
-{{- define "opentelemetry-collector.applyOwnTracesConfig" -}} +{{- define "opentelemetry-collector.applySelfTracesConfig" -}}Likely invalid or redundant comment.
289-299: Verify the filter logic for error logsThe filter processor
filter/non_error_logswith conditionnot IsMatch(body, ".*error.*")is correctly configured to drop non-error logs, keeping only the error logs as intended.Run this script to verify the filter logic:
✅ Verification successful
Filter logic for error logs is correctly implemented ✅
The filter processor is properly configured to retain only error logs by dropping non-error logs, and this implementation is consistently validated in the test suite.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the filter logic for error logs # Test: Search for similar filter patterns in the codebase rg -A 2 'IsMatch\(body.*error'Length of output: 501
197-222:⚠️ Potential issueUse the provided API key for infrastructure data
The self telemetry configuration is incorrectly using
SIGNOZ_SELF_TELEMETRY_API_KEY. As per the PR comments, we should use the provided API key for infrastructure data.Apply this diff to fix the API key usage:
headers: - "signoz-access-token": "${env:SIGNOZ_SELF_TELEMETRY_API_KEY}" + "signoz-access-token": "${env:SIGNOZ_API_KEY}"Likely invalid or redundant comment.
charts/k8s-infra/values.yaml (2)
114-114: LGTM! Setting logs collection from end prevents duplicate data.The change to start collecting logs from the end aligns with the PR objectives and helps prevent duplicate data ingestion when the collector restarts.
93-111:⚠️ Potential issueSecurity: Update default values for secure communication.
The default values for
insecureandinsecureSkipVerifyshould be set tofalseto enforce secure communication by default.Apply this diff to update the default values:
selfTelemetry: # OTLP HTTP endpoint to send own telemetry data to. endpoint: "" # Whether to use insecure mode. - insecure: true + insecure: false # Whether to skip verifying the certificate. - insecureSkipVerify: true + insecureSkipVerify: falseLikely invalid or redundant comment.
LGTM
nityanandagohain
dismissed stale reviews from grandwizard28 and prashant-shahi
via
fc7bfbb
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
charts/k8s-infra/tests/otel-agent_self_telemetry_test.yaml (1)
66-69: Enhance error log filtering pattern.The current regex pattern might miss variations of "error". Consider using a case-insensitive pattern:
logs: log_record: - - not IsMatch(body, ".*error.*") + - not IsMatch(body, "(?i).*error.*")charts/k8s-infra/templates/apikey-self-telemetry-secret.yaml (1)
1-14: Consider additional secrets for comprehensive telemetry configuration.Based on the PR objectives, consider creating additional secrets or configmap entries for:
- Log filtering rules to prevent deployment logs
- Error log patterns for selective logging
- HTTPS endpoint configuration
This would provide a more modular and maintainable configuration structure for the telemetry features.
🧰 Tools
🪛 yamllint (1.35.1)
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
charts/k8s-infra/templates/apikey-self-telemetry-secret.yaml(1 hunks)charts/k8s-infra/tests/otel-agent_self_telemetry_enable_otlp_test.yaml(1 hunks)charts/k8s-infra/tests/otel-agent_self_telemetry_test.yaml(1 hunks)
🧰 Additional context used
📓 Learnings (1)
charts/k8s-infra/tests/otel-agent_self_telemetry_test.yaml (1)
Learnt from: nityanandagohain
PR: SigNoz/charts#595
File: charts/k8s-infra/tests/otel-agent_self_telemetry_test.yaml:11-11
Timestamp: 2025-01-15T12:31:10.850Z
Learning: Helm unit test files (.../tests/*.yaml) should use hardcoded values instead of environment variables to ensure deterministic test results and consistent template validation.
🪛 yamllint (1.35.1)
charts/k8s-infra/tests/otel-agent_self_telemetry_enable_otlp_test.yaml
[error] 165-165: no new line character at the end of file
(new-line-at-end-of-file)
charts/k8s-infra/templates/apikey-self-telemetry-secret.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
charts/k8s-infra/tests/otel-agent_self_telemetry_test.yaml
[error] 135-135: no new line character at the end of file
(new-line-at-end-of-file)
🔇 Additional comments (6)
charts/k8s-infra/tests/otel-agent_self_telemetry_test.yaml (2)
71-79: Well implemented log collection configuration!The configuration correctly:
- Sets
start_at: endto prevent duplicate data- Includes container parsing
- Captures the full log path
11-11:⚠️ Potential issueUpdate endpoint format to use HTTPS protocol.
The endpoint format should include the HTTPS protocol since port 443 is being used:
- endpoint: myhost:443 + endpoint: https://myhostLikely invalid or redundant comment.
charts/k8s-infra/tests/otel-agent_self_telemetry_enable_otlp_test.yaml (1)
50-56: Well-structured OTLP exporter configuration!The configuration correctly:
- Uses environment variables for endpoint and security settings
- Includes proper TLS configuration
- Handles sensitive headers appropriately
charts/k8s-infra/templates/apikey-self-telemetry-secret.yaml (3)
1-1: LGTM! Well-structured conditional creation.The condition correctly handles both scenarios: using an existing secret or creating a new one.
🧰 Tools
🪛 yamllint (1.35.1)
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
2-11: LGTM! Well-configured Secret metadata.The configuration follows best practices:
- Proper Helm hooks ensure the secret exists before dependent resources
- Clear naming with the
-self-telemetry-apikey-secretsuffix- Appropriate delete policy to handle upgrades
12-13: Verify API key validation in the parent chart.The Secret data is correctly configured with proper encoding. However, since this is sensitive data:
Please ensure there's validation for the API key format in the parent chart's values schema. Run this script to check:
These are changes on top of #540
Main changes
httpsAlso logs should be read from end right? so I changed beginning to end otherwise duplicate data will be inserted.
Summary by CodeRabbit
Release Notes
New Features
Configuration Changes
Testing
Maintenance
.gitignoreto excluderendered/directory.0.12.0to0.12.1.