-
Notifications
You must be signed in to change notification settings - Fork 510
Add dedot to templates #2830
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
Add dedot to templates #2830
Conversation
|
Pinging @elastic/elastic-agent-data-plane (Team:Elastic-Agent-Data-Plane) |
|
I don't see anything obviously wrong but I haven't really touched this repository. Is there a change log you need to update? I would ping someone from the integrations side on Slack to look at this if you haven't already. |
|
Alright, finally managed to test this. |
|
/test |
packages/docker/manifest.yml
Outdated
| - os_system | ||
| conditions: | ||
| kibana.version: ^8.0.0 | ||
| kibana.version: ^8.2.0 |
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.
Does this PR contain anything that can't work with 8.0? If not, I'd keep the constraint on the lower version
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 skip_major config opt is new to 8.2, yah.
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.
Looking at this, would it make sense to split both changes? We could release first a version with the fix for dedotting, so this is fixed for current users, and then another version, targetting 8.2 with the skip_major config.
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.
@fearful-symmetry thoughts about releasing this as two different issues?
| release: ga | ||
| description: | | ||
| Container labels | ||
| - name: container.cpu.usage |
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'm not fully sure but shouldn't those ECS fields be in the ecs.yml 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.
I left one question but everything looks good to me
| - name: container.cpu.usage | ||
| type: scaled_float | ||
| format: percent | ||
| description: ECS field for container CPU usage |
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.
You can import definitions of ECS fields with external: ecs. Consider doing this at least for all the ECS fields added in this PR.
| - name: container.cpu.usage | |
| type: scaled_float | |
| format: percent | |
| description: ECS field for container CPU usage | |
| - name: container.cpu.usage | |
| external: ecs |
ECS version used is defined in _dev/build/build.yml.
🌐 Coverage report
|
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.
Looks good, wondering if this should be released as two different things, and as a new minor.
Co-authored-by: Jaime Soriano Pastor <jaime.soriano@elastic.co>
Co-authored-by: Jaime Soriano Pastor <jaime.soriano@elastic.co>
|
There is this open question #2830 (comment), it LGTM in either case, but I would like to have your thoughts on this. |
|
@jsoriano yah, how about I do the |
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.
👍
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.
LGTM
What does this PR do?
This adds the newI also noticed that the actual entry for theskip_majoroption to the docker/diskio integration.DeDotvalue was missing from most of the handlebars files, so I added those in as well.This also fixes some missing fields in the mapping that were breaking CI.
Checklist
changelog.ymlfile.