-
Notifications
You must be signed in to change notification settings - Fork 540
Add docker_otel_input package #17372
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
This package implements a Docker Stats input using the OpenTelemetry Collector's dockerstats receiver, following the pattern established in PR #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: #15731 Co-authored-by: William Easton <[email protected]>
Co-authored-by: Jaime Soriano Pastor <[email protected]>
|
@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? It looks like some of the ways we're configuring collection are no longer available and use the Can you review the input package for other discrepancies? |
Vale Linting ResultsSummary: 2 warnings, 4 suggestions found
|
| File | Line | Rule | Message |
|---|---|---|---|
| packages/docker_input_otel/_dev/build/docs/README.md | 7 | Elastic.Latinisms | Latin terms and abbreviations are a common source of confusion. Use 'using' instead of 'via'. |
| packages/docker_input_otel/docs/README.md | 7 | Elastic.Latinisms | Latin terms and abbreviations are a common source of confusion. Use 'using' instead of 'via'. |
💡 Suggestions (4)
| File | Line | Rule | Message |
|---|---|---|---|
| packages/docker_input_otel/_dev/build/docs/README.md | 11 | Elastic.Versions | Use 'or later' instead of 'or higher' when referring to versions. |
| packages/docker_input_otel/_dev/build/docs/README.md | 20 | Elastic.WordChoice | Consider using 'refer to (if it's a document), view (if it's a UI element)' instead of 'see', unless the term is in the UI. |
| packages/docker_input_otel/docs/README.md | 11 | Elastic.Versions | Use 'or later' instead of 'or higher' when referring to versions. |
| packages/docker_input_otel/docs/README.md | 20 | Elastic.WordChoice | Consider using 'refer to (if it's a document), view (if it's a UI element)' instead of 'see', unless the term is in the UI. |
The Vale linter checks documentation changes against the Elastic Docs style guide.
To use Vale locally or report issues, refer to Elastic style guide for Vale.
|
Claude finished @strawgate's task in 2m 52s —— View job Reviewing Docker OTel Input Package
SummaryI've compared this PR against merged OTel input packages (
|
| Field | This PR | nginx_input_otel | kubeletstats_input_otel | Upstream |
|---|---|---|---|---|
collection_interval |
✅ (as period) |
✅ | ✅ | ✅ |
endpoint |
✅ | ✅ | ✅ | ✅ |
initial_delay |
❌ Missing | ✅ | ✅ | ✅ |
timeout |
✅ | N/A | N/A | ✅ |
api_version |
✅ | N/A | N/A | ✅ |
excluded_images |
✅ | N/A | N/A | ✅ |
metrics (yaml) |
❌ Missing | N/A | ✅ | ✅ |
container_labels_to_metric_labels |
✅ | N/A | N/A | ✅ |
env_vars_to_metric_labels |
✅ | N/A | N/A | ✅ |
provide_per_core_cpu_metrics |
N/A | N/A | ❌ Removed | |
source.license |
❌ Missing | ✅ | ✅ | N/A |
type: duration for time fields |
❌ type: text |
✅ | ✅ | N/A |
📁 Missing Package Structure Elements
Based on other merged packages, consider adding:
_dev/build/build.yml- Build configuration (present inprometheus_input,sql_input, etc.)- System tests - More comprehensive test policies (see
kubeletstats_input_otelpattern with multiple test configs)
✅ What's Done Well
- Good documentation in
docs/README.md - Core configuration options are covered
- Package structure follows the basic pattern
- Uses
otelcolinput type correctly - Includes resourcedetection processor in the pipeline
Recommended Changes
- Add
metricsyaml field to enable/disable specific metrics - Remove
provide_per_core_cpu_metrics(deprecated upstream) - Add
initial_delayfield for consistency - Change
type: texttotype: durationfor time-based fields - Add
source.license: "Elastic-2.0"to manifest - Consider renaming
periodtocollection_intervalto match upstream naming
--- |
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The package structure follows the OTel input pattern well. A few items need attention:
Policy testing: The test-docker-stats.yml needs a corresponding .expected file to verify the rendered policy output (see hostmetrics_input_otel and iis_input_otel for examples).
Consistency with other packages: The period variable is marked required but has a fallback in the template. Other OTel input packages use required: false with a default value, which also simplifies the template.
Please also verify that CI passes with the policy tests once the expected file is added.
Why is Claude responding? | Type @claude to interact further
Give us feedback! React with 🚀 if perfect, 👍 if helpful, 👎 if not.
| changes: | ||
| - description: Initial draft of Docker Stats OpenTelemetry input package | ||
| type: enhancement | ||
| link: https://github.com/elastic/integrations/pull/15739 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟡 MEDIUM Changelog links to wrong PR
Why: The changelog references closed PR #15739 instead of this PR #17372, which will cause confusion in release notes
| link: https://github.com/elastic/integrations/pull/15739 | |
| link: https://github.com/elastic/integrations/pull/17372 |
Why is Claude responding? | Type @claude to interact further
Give us feedback! React with 🚀 if perfect, 👍 if helpful, 👎 if not.
| kibana: | ||
| version: "^9.2.0" | ||
| elastic: | ||
| subscription: "basic" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
⚪ LOW Missing source.license field
Why: Other OTel input packages include source.license to explicitly specify the license
| subscription: "basic" | |
| elastic: | |
| subscription: "basic" | |
| source: | |
| license: "Elastic-2.0" |
Why is Claude responding? | Type @claude to interact further
Give us feedback! React with 🚀 if perfect, 👍 if helpful, 👎 if not.
| type: text | ||
| title: Collection Interval | ||
| description: How often to collect metrics (e.g., 10s, 1m). Default is 60s. | ||
| required: true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟡 MEDIUM Period should have a default, not be required
Why: The template has a fallback default (60s), suggesting period is intended to be optional. Other OTel input packages use required: false with a default value. Having required: true means the template's else clause is unreachable code.
| required: true | |
| description: How often to collect metrics (e.g., 10s, 1m). Default is 60s. | |
| required: false | |
| default: 60s |
Why is Claude responding? | Type @claude to interact further
Give us feedback! React with 🚀 if perfect, 👍 if helpful, 👎 if not.
| receivers: | ||
| - docker_stats | ||
| processors: | ||
| - resourcedetection No newline at end of file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
⚪ LOW Missing trailing newline
Why: Files should end with a newline per POSIX standard and common convention
| - resourcedetection | |
| - resourcedetection |
Why is Claude responding? | Type @claude to interact further
Give us feedback! React with 🚀 if perfect, 👍 if helpful, 👎 if not.
| description: Data stream namespace. | ||
| - name: '@timestamp' | ||
| type: date | ||
| description: Event timestamp. No newline at end of file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
⚪ LOW Missing trailing newline
Why: Files should end with a newline per POSIX standard
| description: Event timestamp. | |
| description: Event timestamp. |
Why is Claude responding? | Type @claude to interact further
Give us feedback! React with 🚀 if perfect, 👍 if helpful, 👎 if not.
| @@ -0,0 +1,3 @@ | |||
| vars: | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟡 MEDIUM Missing .expected file for policy test
Why: Other OTel input packages (hostmetrics_input_otel, iis_input_otel, etc.) include .expected files that verify the rendered policy output. Without this, policy rendering issues won't be caught.
Why is Claude responding? | Type @claude to interact further
Give us feedback! React with 🚀 if perfect, 👍 if helpful, 👎 if not.
| @@ -0,0 +1,48 @@ | |||
| receivers: | |||
| docker_stats: | |||
| {{#if period}} | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
⚪ LOW Simplify template after adding default to manifest
Why: If the manifest provides a default value for period, this conditional becomes unnecessary since period will always have a value.
| {{#if period}} | |
| collection_interval: {{period}} |
Why is Claude responding? | Type @claude to interact further
Give us feedback! React with 🚀 if perfect, 👍 if helpful, 👎 if not.
| description: Mapping of environment variables to metric labels (YAML format) | ||
| required: false | ||
| owner: | ||
| github: elastic/obs-infraobs-integrations |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing to update the .github/CODEOWNERS file , and probably this should be owned for the time being by elastic/ecosystem ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was present on the prior PR, just synced the older changes on top of this one.
However, I added the owner as packages/docker_input_otel @elastic/obs-infraobs-integrations since it seems to match the remaining otel input packages.
Do you know how this is decided?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be possible to add some system test too?
And if it is possible, please create a new sample_event.json file via elastic-package test system --generate command.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could it be used for those system tests the same approach that it is used in the docker package ?
https://github.com/elastic/integrations/tree/main/packages/docker/_dev/deploy/docker
|
|
||
| This integration uses the OpenTelemetry dockerstats receiver which is currently in **alpha** stability. The configuration and metrics may change between versions. | ||
|
|
||
| ## Metrics |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Usually, we are adding a paragraph pointing out to the upstream configuration instead of adding these details here to avoid being outdated with respect to the upstream documentation.
Example:
https://github.com/elastic/integrations/tree/main/packages/nginx_input_otel/docs#metrics-reference
| ## Compatibility | ||
|
|
||
| This integration uses the OpenTelemetry dockerstats receiver which is currently in **alpha** stability. The configuration and metrics may change between versions. | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be interesting to add a Configuration section as in the other input otel packages pointing to the actual upstream documentation.
Example:
|
@claude Review the PR for any discrepancies, any existing comments, compare against existing packages and make a TODO list with all the action items. |
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR adds a well-structured Docker Stats OpenTelemetry input package. The package passes lint and builds successfully.
Outstanding items from existing reviews:
- @mrodm requested documentation changes to point to upstream OTel receiver docs rather than duplicating details inline (threads on
_dev/build/docs/README.mdlines 17, 22) - The
periodvariable usesrequired: truewith a default value, which differs from other OTel input packages that userequired: falsewith a default
Notes:
- The
source.licensefield is already present (lines 5-6 in manifest.yml) - an existing comment suggested it was missing but this appears to be addressed - CODEOWNERS is correctly updated with
@elastic/obs-infraobs-integrationsownership - Policy tests with expected files are now included and render correctly
Why is Claude responding? | Type @claude to interact further
Give us feedback! React with 🚀 if perfect, 👍 if helpful, 👎 if not.
💚 Build Succeeded
History
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The package structure follows the established OTel input package patterns. The build and lint checks pass locally.
Existing feedback that still needs attention:
periodvariable hasrequired: truewith a default value. Other OTel input packages userequired: falsesince the default makes it optional.- Documentation should include a Configuration section linking to upstream dockerstatsreceiver configuration to avoid duplication and stay current.
The source.license field and changelog link have been addressed since earlier comments. CODEOWNERS has been updated as discussed.
Why is Claude responding? | Type @claude to interact further
Give us feedback! React with 🚀 if perfect, 👍 if helpful, 👎 if not.
This package implements a Docker Stats input using the OpenTelemetry
Collector's dockerstats receiver, following the pattern established
in PR #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: #15731