[Draft] Add docker_otel_input package#15739
Conversation
This package implements a Docker Stats input using the OpenTelemetry Collector's dockerstats receiver, following the pattern established in PR elastic#14315. Key features: - Type: integration with otelcol input (not content package) - Configurable collection interval, endpoint, and filtering - Comprehensive field definitions for container metrics - Full documentation and test policy Resolves: elastic#15731 Co-authored-by: William Easton <[email protected]>
andrewkroh
added
New Integration
| processors: | ||
| resourcedetection: | ||
| # Modify the list of detectors to match the cloud environment | ||
| detectors: [env, system, gcp, ec2, azure] |
There was a problem hiding this comment.
This needs to be configurable as long as it lives here. In the long term this neeeds to not be in each package.
There was a problem hiding this comment.
In the long term this neeeds to not be in each package.
I guess we could inject this from Fleet, and make it configurable there.
There was a problem hiding this comment.
Does this actually need to be configurable for now?
There was a problem hiding this comment.
Should this just be a variable that can be configured by the user when enabling the integration?
Other integrations so far only seem to use system, so we could use system as a default, but allow it to be modified when enabling the integration. Would that be acceptable?
There was a problem hiding this comment.
@strawgate, @cmacknz , @jsoriano
I have a change ready locally that allows the detectors to be configurable. (See attached image, but ignore the comma separated description, that will be fixed to reflect the current UX)
Is this what is expected here and am I ok to push it? Or is the expectation that this should be done in a different way ?
Co-authored-by: Jaime Soriano Pastor <[email protected]>
Fixes #243720 ## Summary Integration input health reports otelcol inputs with `receiver id` instead of input type `otelcol`. Currently the input health show input name "otelcol" for any receiver, making it difficult to diagnose which input has issues in case of multiple inputs. This PR addresses it, showing the receiver id instead of the generic type. Note that this change is done only for "otelcol" inputs and not for any other type of inputs, they'll keep showing the type instead; the link ref stays the same, it's just the text that has been changed. ### Before <img width="951" height="638" alt="Screenshot 2025-11-24 at 15 57 20" src="https://github.com/user-attachments/assets/9e67c70b-4e73-45c6-8bc4-ba0c5de70b91" /> ### After <img width="1771" height="996" alt="Screenshot 2025-11-24 at 15 51 36" src="https://github.com/user-attachments/assets/84a3ef3f-8895-405c-8417-dda772964d09" /> ### Testing - Locally build this package: elastic/integrations#15739 - Install it into Fleet with ``` curl -XPOST -H 'content-type: application/zip' -H 'kbn-xsrf: true' http://localhost:5601/<PATH>/api/fleet/epm/packages -u elastic:changeme --data-binary @docker_otel_input-0.2.0.zip ``` - Add it to an agent policy and enroll an agent into that policy - Verify the input health in agent view. The input name should correspond to the receiver id and the link should correctly navigate to the logs. ### Checklist - [ ] [Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html) was added for features that require explanation or tutorials - [ ] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios
…ic#244013) Fixes elastic#243720 ## Summary Integration input health reports otelcol inputs with `receiver id` instead of input type `otelcol`. Currently the input health show input name "otelcol" for any receiver, making it difficult to diagnose which input has issues in case of multiple inputs. This PR addresses it, showing the receiver id instead of the generic type. Note that this change is done only for "otelcol" inputs and not for any other type of inputs, they'll keep showing the type instead; the link ref stays the same, it's just the text that has been changed. ### Before <img width="951" height="638" alt="Screenshot 2025-11-24 at 15 57 20" src="https://github.com/user-attachments/assets/9e67c70b-4e73-45c6-8bc4-ba0c5de70b91" /> ### After <img width="1771" height="996" alt="Screenshot 2025-11-24 at 15 51 36" src="https://github.com/user-attachments/assets/84a3ef3f-8895-405c-8417-dda772964d09" /> ### Testing - Locally build this package: elastic/integrations#15739 - Install it into Fleet with ``` curl -XPOST -H 'content-type: application/zip' -H 'kbn-xsrf: true' http://localhost:5601/<PATH>/api/fleet/epm/packages -u elastic:changeme --data-binary @docker_otel_input-0.2.0.zip ``` - Add it to an agent policy and enroll an agent into that policy - Verify the input health in agent view. The input name should correspond to the receiver id and the link should correctly navigate to the logs. ### Checklist - [ ] [Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html) was added for features that require explanation or tutorials - [ ] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios
…ic#244013) Fixes elastic#243720 ## Summary Integration input health reports otelcol inputs with `receiver id` instead of input type `otelcol`. Currently the input health show input name "otelcol" for any receiver, making it difficult to diagnose which input has issues in case of multiple inputs. This PR addresses it, showing the receiver id instead of the generic type. Note that this change is done only for "otelcol" inputs and not for any other type of inputs, they'll keep showing the type instead; the link ref stays the same, it's just the text that has been changed. ### Before <img width="951" height="638" alt="Screenshot 2025-11-24 at 15 57 20" src="https://github.com/user-attachments/assets/9e67c70b-4e73-45c6-8bc4-ba0c5de70b91" /> ### After <img width="1771" height="996" alt="Screenshot 2025-11-24 at 15 51 36" src="https://github.com/user-attachments/assets/84a3ef3f-8895-405c-8417-dda772964d09" /> ### Testing - Locally build this package: elastic/integrations#15739 - Install it into Fleet with ``` curl -XPOST -H 'content-type: application/zip' -H 'kbn-xsrf: true' http://localhost:5601/<PATH>/api/fleet/epm/packages -u elastic:changeme --data-binary @docker_otel_input-0.2.0.zip ``` - Add it to an agent policy and enroll an agent into that policy - Verify the input health in agent view. The input name should correspond to the receiver id and the link should correctly navigate to the logs. ### Checklist - [ ] [Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html) was added for features that require explanation or tutorials - [ ] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios
|
Hi! We just realized that we haven't looked into this PR in a while. We're sorry! We're labeling this issue as |
|
@claude can you please review this PR for me? |
|
Claude finished @strawgate's task in 2m 37s —— View job Review Complete
I've submitted my review with a COMMENT outcome. See the review for details. Summary:
Why is Claude responding? | Type Give us feedback! React with 🚀 if perfect, 👍 if helpful, 👎 if not. |
| - name: api_version | ||
| type: text | ||
| title: API Version | ||
| description: Docker API version to use |
There was a problem hiding this comment.
i think we should default to api version 1.44 maybe?
|
@claude can you set the api version to 1.44? |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
The package structure follows the established otelcol input pattern from PR #14315, but there are a few issues to address:
Blocking Issues:
- Deprecated configuration option: The
provide_per_core_cpu_metricsoption was removed from the dockerstats receiver in ScraperV2. This option and its template handling should be removed. To enable per-core CPU metrics, users should configure themetricsblock to enablecontainer.cpu.usage.percpuinstead.
Other Items:
-
Missing policy test
.expectedfile: Other otelcol input packages (httpcheck_otel, hostmetrics_input_otel) include.expectedfiles alongside their policy test YAML files. Consider addingtest-docker-stats.expectedto verify the rendered configuration. -
Version consistency: The package version is 0.2.0 but this is the initial release. Convention in this repo is to start at 0.1.0.
Why is Claude responding? | Type @claude to interact further
Give us feedback! React with 🚀 if perfect, 👍 if helpful, 👎 if not.
|
Claude finished @strawgate's task in 2m 5s —— View job API Version Set to 1.44
Change: Added Why is Claude responding? | Type Give us feedback! React with 🚀 if perfect, 👍 if helpful, 👎 if not. |
|
@claude Can you review the PR and check if all the comments have been addressed |
There was a problem hiding this comment.
The package has several issues that need to be addressed before merge:
Critical Issues
1. Package Name/Directory Mismatch
The directory is named docker_input_otel but the manifest declares name: docker_otel_input. This should be consistent - rename either the directory or the package name. The existing pattern in hostmetrics_input_otel uses matching names.
2. Incorrect Configuration Format for Label Mappings
The container_labels_to_metric_labels and env_vars_to_metric_labels fields are being treated incorrectly throughout:
- Manifest: Declares them as
type: yaml(correct) - Test data: Provides them as arrays instead of maps
- Template: Outputs them inline which produces comma-separated strings
- OTel receiver expectation: These must be
map[string]string(source_label: metric_label)
The current configuration produces output like:
container_labels_to_metric_labels: com.docker.compose.service,com.docker.compose.projectBut dockerstats receiver expects:
container_labels_to_metric_labels:
com.docker.compose.service: compose_service
com.docker.compose.project: compose_projectThis requires changes to:
- Test input files to use map format
- Template rendering to iterate over map entries
- Expected output files to reflect the correct format
Why is Claude responding? | Type @claude to interact further
Give us feedback! React with 🚀 if perfect, 👍 if helpful, 👎 if not.
packages/docker_input_otel/_dev/test/policy/test-docker-stats.yml
Outdated
Show resolved
Hide resolved
packages/docker_input_otel/_dev/test/policy/test-docker-stats.yml
Outdated
Show resolved
Hide resolved
packages/docker_input_otel/_dev/test/policy/test-docker-stats.expected
Outdated
Show resolved
Hide resolved
packages/docker_input_otel/_dev/test/policy/test-docker-stats.yml
Outdated
Show resolved
Hide resolved
packages/docker_input_otel/_dev/test/policy/test-default.expected
Outdated
Show resolved
Hide resolved
packages/docker_input_otel/_dev/test/policy/test-docker-stats.expected
Outdated
Show resolved
Hide resolved
| receivers: | ||
| - docker_stats | ||
| processors: | ||
| - resourcedetection |
There was a problem hiding this comment.
💬 NITPICK Missing trailing newline
Why: Files should end with a newline for POSIX compliance.
| - resourcedetection | |
| - resourcedetection |
Why is Claude responding? | Type @claude to interact further
Give us feedback! React with 🚀 if perfect, 👍 if helpful, 👎 if not.
I had to add 1.44 on my setup because it installed Docker v29, which requires at least 1.44. When no version is specified, it was defaulting to 1.25, causing the docker stats receiver to fail. IMO, I guess we could make 1.44 default since it has been present in Docker Engine since v25, which has been released in January 2024. ref: https://docs.docker.com/engine/release-notes/25.0/#2500 However, introducing this version will have drawbacks in the sense we will have to come back and update the repo once more at a later point either to remove the default and bump the version. Since this will be a preview version, I guess we could add the default to align with what docker is mandating, but what are your thoughts? |
|
@claude how does this compare to the other feedback given to other input package PRs? as well as what has merged into main? any important discrepancies? |
|
Claude finished @strawgate's task in 2m 33s —— View job Comparing to Other OTel Input Package PRs
SummaryI compared this PR against 6 merged OTel input packages ( ✅ What This PR Does Well (Aligned with Merged Packages)
|
| Issue | This PR | Merged Packages | Severity |
|---|---|---|---|
| resourcedetection detectors | [env, system, gcp, ec2, azure] |
All use ["system"] only |
🟡 MEDIUM |
Missing _dev/build/docs/README.md |
Not present | Present in nginx, iis, hostmetrics, kubeletstats | 🟡 MEDIUM |
Missing show_user hints |
Only period has implied show_user |
All merged packages explicitly set show_user: true/false on every var |
⚪ LOW |
| Icon title | "Docker logo" |
nginx/iis use "Sample logo", hostmetrics uses "System OTel logo" |
⚪ LOW (fine as is) |
| Verbose docs/README.md | 109 lines, detailed | hostmetrics: 20 lines, concise with upstream links | ⚪ LOW |
📋 Common Feedback Patterns Across OTel Input PRs
Looking at feedback on open PRs (Redis, MySQL, PostgreSQL), I see these recurring themes that this PR has already addressed:
- Secrets handling bug (Fleet Server TSDB enabled release for kafka #6277) - Not applicable here (no password fields)
- Package name/directory mismatch - ✅ Fixed:
docker_input_otelmatches folder - SVG security - Not flagged (logo appears clean)
- TLS configuration patterns - Not applicable here
🔴 Outstanding Item (Unresolved Review Thread)
There's one unresolved thread from @cmacknz about making the resourcedetection detectors configurable. While you've been ignoring this per earlier instructions, I should note:
- All merged packages use
detectors: ["system"]only (nginx, iis, prometheus, statsd, hostmetrics) - One exception:
kubeletstats_input_otelhas no resourcedetection processor at all - This PR uses
[env, system, gcp, ec2, azure]with atimeout: 2s- the cloud detectors add latency
@jmmcorreia commented that they have a local change ready to make detectors configurable. If that's the direction, it would align with hostmetrics_input_otel which makes system_config configurable.
🎯 Recommendations
-
Decide on resourcedetection: Either:
- Change to
["system"]only (like nginx, iis, prometheus, statsd) — simplest - Make it configurable (like @jmmcorreia proposed) — most flexible
- Keep as-is with cloud detectors — but document the latency implications
- Change to
-
Add
_dev/build/docs/README.md: All merged packages have this (can be empty or minimal) -
Add
show_userhints: For consistency, explicitly addshow_user: falsetoendpoint,excluded_images,timeout,api_version,provide_per_core_cpu_metrics,container_labels_to_metric_labels,env_vars_to_metric_labels
--- |
Why is Claude responding? | Type @claude to interact further
Give us feedback! React with 🚀 if perfect, 👍 if helpful, 👎 if not.
There was a problem hiding this comment.
The provide_per_core_cpu_metrics configuration option has been removed from the OpenTelemetry dockerstats receiver as part of the ScraperV2 migration. This option needs to be removed from the manifest, template, tests, and documentation.
If per-core CPU metrics are needed, the receiver now supports enabling specific metrics through the metrics configuration block:
receivers:
docker_stats:
metrics:
container.cpu.usage.percpu:
enabled: trueThere is also an ongoing discussion about making the resourcedetection detectors configurable (see existing thread at line 32 of input.yml.hbs).
Why is Claude responding? | Type @claude to interact further
Give us feedback! React with 🚀 if perfect, 👍 if helpful, 👎 if not.
packages/docker_input_otel/_dev/test/policy/test-docker-stats.expected
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
This draft PR implements a Docker Stats input package using the OTel dockerstats receiver. The implementation follows established patterns from other otel input packages.
Unresolved discussions:
- The resourcedetection detector configuration (line 32 in input.yml.hbs) is under active discussion. A team member has a local change ready to make detectors configurable - this should be resolved before merging.
No new issues found. The package structure, template logic, and test policies look correct. The YAML map rendering with multiline strings works properly based on the expected test outputs.
Why is Claude responding? | Type @claude to interact further
Give us feedback! React with 🚀 if perfect, 👍 if helpful, 👎 if not.
|
@claude Can you review again the PR, compare against other existing packages and compare against all the feedback in this thread ? |
💚 Build Succeeded
History
|
|
@claude Can you review again the PR, compare against other existing packages and compare against all the feedback in this thread ? |
|
Claude finished @strawgate's task in 2m 34s —— View job PR Review: Comparing Against Other Packages and Thread Feedback
SummaryI've submitted a detailed review comparing this PR against all merged OTel input packages. Key Findings: ✅ Addressed Items: Package naming, source license, version, duration types, show_user hints, policy tests, api_version default — all correct
⚪ Minor: Changelog is missing the Package Validation: ✅ Why is Claude responding? | Type Give us feedback! React with 🚀 if perfect, 👍 if helpful, 👎 if not. |
There was a problem hiding this comment.
Comprehensive Review: Comparison Against Merged OTel Input Packages
I've compared this PR against all merged OTel input packages (hostmetrics_input_otel, nginx_input_otel, kubeletstats_input_otel, statsd_input_otel, prometheus_input_otel) and all feedback in this thread. Here's the current state:
✅ Addressed Issues (Already Fixed)
| Issue | Status |
|---|---|
Package name/directory: docker_input_otel |
✅ Correct |
source.license: "Elastic-2.0" |
✅ Present |
Version 0.1.0 |
✅ Correct |
period uses type: duration with default: 60s |
✅ Correct |
timeout uses type: duration |
✅ Correct |
All variables have show_user hints |
✅ Present |
_dev/build/docs/README.md present |
✅ Added |
Policy test .expected files present |
✅ Added |
| YAML vars render correctly (multiline block) | ✅ Working |
api_version default 1.44 |
✅ Added |
⚠️ Open Items / Discrepancies
1. resourcedetection detectors (Unresolved Discussion)
Current: [env, system, gcp, ec2, azure] with timeout: 2s
Other packages: ["system"] only (nginx, statsd, prometheus, hostmetrics)
Exception: kubeletstats_input_otel has no resourcedetection processor at all
@jmmcorreia has indicated they have a change ready to make this configurable. This seems like the right approach since:
- It aligns with @cmacknz's original request
hostmetrics_input_otelalready makessystem_configconfigurable- Cloud detectors add latency via the 2s timeout
Recommendation: Consider making detectors configurable (like hostmetrics's system_config) or simplify to ["system"] to match other packages.
2. Missing # newer versions go on top comment in changelog
Current: Starts directly with version entry
Other packages: hostmetrics, kubeletstats, etc. all include this header
3. Template uses deprecated approach for per-core CPU
The current approach uses a provide_per_core_cpu_metrics boolean to toggle:
metrics:
container.cpu.usage.percpu:
enabled: {{provide_per_core_cpu_metrics}}This is actually correct for the current OTel dockerstats receiver. The old config option provide_per_core_cpu_metrics was deprecated, but the receiver now uses the standard metrics block which is what this package does. However, the naming of the variable (provide_per_core_cpu_metrics) is misleading since users familiar with the old receiver might think this is the deprecated option.
Recommendation: Consider renaming to enable_percpu_metrics or similar to avoid confusion with the deprecated option name.
📋 Summary
The package is in good shape overall. The main outstanding item is the resourcedetection configuration discussion with @cmacknz, @jsoriano, and @jmmcorreia. The current implementation with cloud detectors works but diverges from other packages.
Packages passing validation: ✅ elastic-package check passes
Ready for merge? Pending decision on resourcedetection approach.
Why is Claude responding? | Type @claude to interact further
Give us feedback! React with 🚀 if perfect, 👍 if helpful, 👎 if not.
| @@ -0,0 +1,5 @@ | |||
| - version: "0.1.0" | |||
There was a problem hiding this comment.
⚪ LOW Missing changelog header comment
Why: Other OTel input packages include a '# newer versions go on top' comment for consistency.
| - version: "0.1.0" | |
| # newer versions go on top | |
| - version: "0.1.0" |
Why is Claude responding? | Type @claude to interact further
Give us feedback! React with 🚀 if perfect, 👍 if helpful, 👎 if not.
|
moved to #17372 |
7 participants
This package implements a Docker Stats input using the OpenTelemetry
Collector's dockerstats receiver, following the pattern established
in PR #14315.
Key features:
Resolves: #15731