Otel collector integration #519
Conversation
|
Mateusz seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
| dependencies: | ||
| - name: opentelemetry-collector | ||
| alias: otel-collector | ||
| version: "0.115.0" |
There was a problem hiding this comment.
Pull request overview
This pull request adds OpenTelemetry Collector integration to the Netdata Helm chart, enabling the collection of logs from Kubernetes pods and forwarding them to a netdata instance via OTLP protocol.
Changes:
- Added
netdataOpentelemetrydeployment and service for receiving OTLP data - Integrated
opentelemetry-collectoras a subchart dependency for collecting Kubernetes logs - Fixed YAML indentation issues in ingress hosts and child tolerations configurations
Reviewed changes
Copilot reviewed 8 out of 10 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| charts/netdata/values.yaml | Added netdataOpentelemetry configuration section and otel-collector subchart settings; fixed indentation issues |
| charts/netdata/templates/netdata-otel/service.yaml | Created service to expose OTLP endpoint on port 4317 |
| charts/netdata/templates/netdata-otel/secrets.yaml | Created secrets template for OTEL configuration |
| charts/netdata/templates/netdata-otel/persistentvolumeclaim.yaml | Created PVC for OTEL log storage |
| charts/netdata/templates/netdata-otel/deployment.yaml | Created deployment for netdata instance with OTEL plugin enabled |
| charts/netdata/templates/netdata-otel/configmap.yaml | Created configmap template for OTEL configuration |
| charts/netdata/templates/_helpers.tpl | Added helper templates for netdataOpentelemetry configs management |
| charts/netdata/Chart.yaml | Added opentelemetry-collector subchart dependency |
| charts/netdata/Chart.lock | Generated lock file for subchart dependency |
| charts/netdata/charts/opentelemetry-collector-0.115.0.tgz | Added OpenTelemetry Collector subchart package |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| exporters: | ||
| # OTLP gRPC exporter to your endpoint | ||
| otlp: | ||
| endpoint: "{{ .Release.Name }}-otel:4317" |
There was a problem hiding this comment.
The OTLP exporter endpoint references "{{ .Release.Name }}-otel:4317", but the actual service created in templates/netdata-otel/service.yaml is named "{{ template "netdata.name" . }}-otel". This mismatch will cause the OpenTelemetry Collector to fail connecting to the netdata-otel service. The endpoint should be "{{ template "netdata.name" . }}-otel:4317" to match the service name, or use a consistent naming pattern.
| endpoint: "{{ .Release.Name }}-otel:4317" | |
| endpoint: "{{ template "netdata.name" . }}-otel:4317" |
| hostname: "netdata-otel" | ||
| port: "{{ .Values.parent.port }}" | ||
|
|
||
| ## Service responsible for receiving OTEL logs |
There was a problem hiding this comment.
The comment describes the service as "Service responsible for receiving OTEL logs", but the netdata OTEL plugin configuration shows it can handle both metrics and logs (lines 652-664). Consider updating the comment to "Service responsible for receiving OTEL metrics and logs" for accuracy.
| ## Service responsible for receiving OTEL logs | |
| ## Service responsible for receiving OTEL metrics and logs |
| service: | ||
| pipelines: | ||
| # Logs pipeline | ||
| logs: | ||
| receivers: [filelog] | ||
| processors: [memory_limiter, k8sattributes, resourcedetection, batch] | ||
| exporters: [otlp] |
There was a problem hiding this comment.
The PR description states "Adding ability to get otel metrics from the kubernetes", but the OpenTelemetry Collector subchart configuration is set up exclusively for logs collection (filelog receiver with logs pipeline at lines 821-824). While the netdata OTEL endpoint can accept both metrics and logs, the current collector configuration only sends logs. Consider either updating the PR description to accurately reflect that this is primarily for logs collection, or adding metrics collection capabilities if that was the intended functionality.
|
@cubic-dev-ai review this PR |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 10 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| data: | | ||
| [stream] | ||
| enabled = {{ ternary "yes" "no" .Values.parent.enabled }} | ||
| destination = netdata:{{ .Values.service.port }} |
There was a problem hiding this comment.
The stream configuration references netdata:{{ .Values.service.port }} as the destination. This assumes the main netdata service exists and is named 'netdata', but the service is actually named using the template {{ template "netdata.name" . }} which might not always be 'netdata'. Consider using a more robust reference like {{ template "netdata.name" . }}:{{ .Values.service.port }} in the template expression to ensure consistency.
| destination = netdata:{{ .Values.service.port }} | |
| destination = {{ template "netdata.name" . }}:{{ .Values.service.port }} |
| {{- end }} | ||
| spec: | ||
| securityContext: | ||
| fsGroup: 201 |
There was a problem hiding this comment.
The securityContext is set to fsGroup 201, but unlike the parent deployment which also sets runAsUser and runAsGroup to 201, this deployment doesn't specify these fields. For consistency and security best practices, consider adding runAsUser and runAsGroup to match the pattern used in the parent deployment, unless there's a specific reason to omit them.
| fsGroup: 201 | |
| fsGroup: 201 | |
| runAsUser: 201 | |
| runAsGroup: 201 |
|
|
||
| podAnnotations: {} | ||
|
|
||
| dnsPolicy: ClusterFirstWithHostNet |
There was a problem hiding this comment.
The dnsPolicy is set to 'ClusterFirstWithHostNet', but this deployment doesn't set hostNetwork to true (unlike the child deployment which does). The 'ClusterFirstWithHostNet' policy is specifically designed for pods using hostNetwork. Since this deployment doesn't use hostNetwork, consider changing this to 'ClusterFirst' or 'Default' to match the parent deployment pattern which uses 'Default'.
| dnsPolicy: ClusterFirstWithHostNet | |
| dnsPolicy: Default |
| # OpenTelemetry Collector subchart configuration | ||
| # This is an optional component that allows to gather logs from k8s cluster. |
There was a problem hiding this comment.
The comment says 'This is an optional component that allows to gather logs from k8s cluster' but the OpenTelemetry Collector configuration includes receivers, processors, and exporters for logs only. The PR description mentions 'otel metrics', which creates a discrepancy. If this is intended to collect both metrics and logs in the future, the comment should reflect that. If it's only for logs, the PR description should be corrected.
| ports: | ||
| - name: http | ||
| containerPort: {{ tpl (.Values.netdataOpentelemetry.port | toString) . }} | ||
| protocol: TCP |
There was a problem hiding this comment.
The container listens on two ports: port 19999 for the Netdata web interface (configured via NETDATA_LISTENER_PORT) and port 4317 for the OTEL endpoint (configured in otel.yaml). However, only the Netdata web interface port is declared in the ports section. For clarity and completeness, consider also declaring the OTEL port (4317) in the container ports section, even though it's not strictly required. This helps with documentation and service discovery.
| protocol: TCP | |
| protocol: TCP | |
| - name: otel | |
| containerPort: 4317 | |
| protocol: TCP |
Adding ability to get otel metrics from the kubernetes