Skip to content
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.

Sign up for GitHub

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

Jump to bottom

3276 metrics-elastic_agent mapping system.process.cpu.total.time.ms as a date #3284

Merged
merged 4 commits into from
May 9, 2022
Merged

3276 metrics-elastic_agent mapping system.process.cpu.total.time.ms as a date #3284

merged 4 commits into from
May 9, 2022

Conversation

AndersonQ
Copy link
Member

What does this PR do?

It changes some of the CPU metrics mappings for the Elsatic Agent. They were date, when should be long.

Checklist

  • I have reviewed tips for building integrations and this pull request is aligned with them.
  • I have verified that all data streams collect metrics or logs.
  • I have added an entry to my package's changelog.yml file.
  • I have verified that Kibana version constraints are current according to guidelines.

Related issues

@AndersonQ AndersonQ added bug Something isn't working Team:Elastic-Agent-Control-Plane Label for the Agent Control Plane team v8.3.0 labels May 5, 2022
@AndersonQ AndersonQ requested a review from a team as a code owner May 5, 2022 14:53
@AndersonQ AndersonQ self-assigned this May 5, 2022
@AndersonQ AndersonQ requested review from ph and aleksmaus May 5, 2022 14:53
@elasticmachine
Copy link

Pinging @elastic/elastic-agent-control-plane (Team:Elastic-Agent-Control-Plane)

@elasticmachine
Copy link

elasticmachine commented May 5, 2022 •
edited
Loading

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview preview

Expand to view the summary

Build stats

  • Start Time: 2022-05-09T09:35:51.839+0000

  • Duration: 13 min 26 sec

Test stats 🧪

Test Results
Failed 0
Passed 30
Skipped 0
Total 30

🤖 GitHub comments

To re-run your PR in the CI, just comment with:

  • /test : Re-trigger the build.

@elasticmachine
Copy link

elasticmachine commented May 5, 2022 •
edited
Loading

🌐 Coverage report

Name Metrics % (covered/total) Diff
Packages 100.0% (0/0) 💚
Files 100.0% (0/0) 💚 3.597
Classes 100.0% (0/0) 💚 3.597
Methods 33.333% (20/60) 👎 -54.847
Lines 100.0% (0/0) 💚 11.043
Conditionals 100.0% (0/0) 💚

andrewkroh
andrewkroh reviewed May 5, 2022
View reviewed changes
packages/elastic_agent/changelog.yml Outdated
Comment on lines 6 to 7
link: |
https://github.com/elastic/integrations/pull/3284
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This format puts a newline at the end of the URL.

  {
    "version": "1.3.2",
    "changes": [
      {
        "description": "Fix some CPU elastic_agent_metrics mapping",
        "type": "bugfix",
        "link": "https://github.com/elastic/integrations/pull/3284\n"
      }
    ]
  }
Suggested change
link: |
https://github.com/elastic/integrations/pull/3284
link: https://github.com/elastic/integrations/pull/3284

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good catch, yaml love..

packages/elastic_agent/data_stream/elastic_agent_metrics/fields/fields.yml Outdated
unit: nanos
description: CPU time consumed by tasks in user (kernel) mode.
- name: percpu
type: object
type: object # shouldn't it be long and nano?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is correct. IIRC it produces counters per CPU like

          "percpu": {
            "1": 652220788827,
            "2": 655150057935,
            "3": 647406003603,
            "4": 650117200711
          }

So these are currently relying on default dynamic mappings which will make them long. Once we support dynamic mappings in the package-spec for fields this can be clarified by stating that percpu.* is a long.

ruflin
ruflin approved these changes May 6, 2022
View reviewed changes
Copy link
Member

@ruflin ruflin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change LGTM but see suggestion for the changelog from @andrewkroh . Most important part of this PR is the change from date to long, lets get it in an release it and have follow up discussions on the counter part.

packages/elastic_agent/changelog.yml Outdated
@@ -1,4 +1,10 @@
# newer versions go on top
- version: "1.3.2"
changes:
- description: Fix some CPU elastic_agent_metrics mapping
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
- description: Fix some CPU elastic_agent_metrics mapping
- description: Fix some CPU elastic_agent_metrics mapping from date to long

ph
ph approved these changes May 6, 2022
View reviewed changes
Copy link
Contributor

@ph ph left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would merge the changes from @ruflin and @andrewkroh but everything else seems fine.

packages/elastic_agent/changelog.yml Outdated
Comment on lines 6 to 7
link: |
https://github.com/elastic/integrations/pull/3284
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good catch, yaml love..

andrewkroh
andrewkroh approved these changes May 6, 2022
View reviewed changes
Copy link
Member

@andrewkroh andrewkroh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but can you please remove the commentary questions before merging.

packages/elastic_agent/data_stream/elastic_agent_metrics/fields/fields.yml Outdated
unit: nanos
description: |
Total CPU time in nanoseconds consumed by all tasks in the cgroup.
- name: stats.user.ns
type: long
metric_type: counter
metric_type: counter # is it correct? ns as a counter?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is it correct? ns as a counter?

The OS accumulates the total amount of time so I think these are all correct as counters.

@AndersonQ AndersonQ merged commit fa8e612 into main May 9, 2022
@AndersonQ AndersonQ deleted the 3276-elastic-agent-metrics branch May 9, 2022 12:50
@hubbleview hubbleview mentioned this pull request Sep 13, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ph ph ph approved these changes

@ruflin ruflin ruflin approved these changes

@andrewkroh andrewkroh andrewkroh approved these changes

@aleksmaus aleksmaus Awaiting requested review from aleksmaus aleksmaus was automatically assigned from elastic/elastic-agent-control-plane

Assignees

@AndersonQ AndersonQ

Labels
bug Something isn't working Integration:Elastic-Agent Team:Elastic-Agent-Control-Plane Label for the Agent Control Plane team v8.3.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

metrics-elastic_agent mapping system.process.cpu.total.time.ms as a date
5 participants
@AndersonQ @elasticmachine @ph @ruflin @andrewkroh