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.

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 improved config/event output options to windows/perfmon metricset #17596

Merged
merged 16 commits into from
Apr 21, 2020

Conversation

narph
Copy link
Contributor

@narph narph commented Apr 7, 2020

What does this PR do?

Works on improving the config, the new config is less repetitive and terser and will get a better overview of the configuration:


- module: windows
  metricsets:
  - perfmon
  period: 10s
  perfmon.group_measurements_by_instance: true
  perfmon.queries:
  - object: 'Process'
    instance: ["svchost*", "conhost*"]
    counters:
    - name: '% Processor Time'
      field: time.processor.pct
      format: "float"
    - name: 'Thread Count'
      field: thread_count
    - name: "IO Read Operations/sec"
// perfmon.counters can still be mapped and will return events but will be deprecated
  perfmon.counters:
  - instance_label: physical_disk.name
    measurement_label: physical_disk.write.per_sec
    query: '\PhysicalDisk(*)\Disk Writes/sec'

output wise, we offer some consistency with the other metricsets (example of event with the new format):

 "windows" : {
            "perfmon" : {
              "instance" : "svchost",
              "metrics" : {
                "time_processor_pct" : 0,
                "io_read_operations_per_sec" : 0,
                "thread_count" : 2
              },
              "object" : "Process"
            }
          },
          "event" : {
            "dataset" : "windows.perfmon",
            "module" : "windows",
            "duration" : 14153100
          }

POC contains the following:

  • new configuration running along with the current configuration, if the user will populate the perfmon.queries list then events will have the new output, else the current one will still be possible. That comes with a deprecation message:
2020-04-10T14:03:57.670+0200	WARN	[cfgwarn]	perfmon/config.go:70	DEPRECATED: perfmon.counters configuration option is deprecated and will be remove in the future major version, we advise using the perfmon.queries configuration option instead Will be removed in version: 8.0
2020-04-10T14:04:00.407+0200	INFO	cfgfile/reload.go:224	Loading of config files completed.
  • the new configuration will contain:
    • option to enter a label/field for the object value, if not entered it will be object
    • no option to enter a label/field for the instance value, it will be instance
    • option to enter a field/label for the counter value, if not entered, one will be generated based on the counter path
  • ex. in the iis/website manifest.yml file on how the light metricsets will have to adapt to the setup

Checklist

  • My code follows the style guidelines of this project
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have made corresponding change to the default configuration files
  • I have added tests that prove my fix is effective or that my feature works
  • I have added an entry in CHANGELOG.next.asciidoc or CHANGELOG-developer.next.asciidoc.

Related issues

TO DO

  • Add tests for the new setup as well
  • Decide on default config/setup, deprecation process etc
  • Rewrite manifest.yml for the iis/webserver light metricset as well

@narph narph self-assigned this Apr 7, 2020
@narph narph added [zube]: In Progress Team:Integrations Label for the Integrations team labels Apr 7, 2020
@elasticmachine
Copy link
Collaborator

Pinging @elastic/integrations (Team:Integrations)

Copy link
Member

@jsoriano jsoriano left a comment

Choose a reason for hiding this comment

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

This is looking good! I will take another look if there are changes for dedotting nested fields.

# - name: 'Disk Writes/sec'
# field: physical_disk.write.per_sec
# format: "float"
# - name: "% Disk Write Time"
Copy link
Member

Choose a reason for hiding this comment

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

Align commented out code as before?

metricbeat/docs/modules/windows.asciidoc Outdated Show resolved Hide resolved
metricbeat/module/windows/perfmon/config.go Outdated Show resolved Hide resolved
metricbeat/module/windows/perfmon/config.go Show resolved Hide resolved
Copy link
Member

@jsoriano jsoriano left a comment

Choose a reason for hiding this comment

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

Thanks for the changes! Some additional suggestions on them.

metricbeat/module/windows/perfmon/config.go Outdated Show resolved Hide resolved
metricbeat/module/windows/perfmon/config.go Outdated Show resolved Hide resolved
metricbeat/module/windows/perfmon/config.go Outdated Show resolved Hide resolved
metricbeat/module/windows/perfmon/config.go Outdated Show resolved Hide resolved
metricbeat/module/windows/perfmon/config.go Show resolved Hide resolved
metricbeat/module/windows/perfmon/config.go Show resolved Hide resolved
@narph narph requested review from jsoriano and exekias April 20, 2020 12:18
@narph narph changed the title POC windows/perfmon metricset new config/output Add improved confic/event output options to windows/perfmon metricset Apr 20, 2020
@narph narph changed the title Add improved confic/event output options to windows/perfmon metricset Add improved config/event output options to windows/perfmon metricset Apr 20, 2020
Comment on lines +32 to +39
perfmon.queries:
# - object: 'Process'
# instance: ["*"]
# counters:
# - name: 'Disk Writes/sec'
# field: physical_disk.write.per_sec
# format: "float"
# - name: "% Disk Write Time"
Copy link
Member

Choose a reason for hiding this comment

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

Nit. Align the comment marks with the previous block:

Suggested change
perfmon.queries:
# - object: 'Process'
# instance: ["*"]
# counters:
# - name: 'Disk Writes/sec'
# field: physical_disk.write.per_sec
# format: "float"
# - name: "% Disk Write Time"
perfmon.queries:
#- object: 'Process'
# instance: ["*"]
# counters:
# - name: 'Disk Writes/sec'
# field: physical_disk.write.per_sec
# format: "float"
# - name: "% Disk Write Time"

Copy link
Member

@jsoriano jsoriano left a comment

Choose a reason for hiding this comment

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

Thanks! Great enhancement!

@narph narph added needs_backport PR is waiting to be backported to other branches. v7.8.0 labels Apr 21, 2020
@narph narph merged commit 1a50329 into elastic:master Apr 21, 2020
@narph narph deleted the perfmon-ga branch April 21, 2020 09:56
narph added a commit to narph/beats that referenced this pull request Apr 21, 2020
…elastic#17596)

* config

* work on perfmon

* progress

* work on perf

* Create tests

* work on documentation

* fix test

* changelog

* work on webserver

* fix test

* add test

* validate

* dynamic mapping

* init format

* work on test

(cherry picked from commit 1a50329)
narph added a commit that referenced this pull request Apr 23, 2020
…#17596) (#17861)

* config

* work on perfmon

* progress

* work on perf

* Create tests

* work on documentation

* fix test

* changelog

* work on webserver

* fix test

* add test

* validate

* dynamic mapping

* init format

* work on test

(cherry picked from commit 1a50329)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Metricbeat Metricbeat needs_backport PR is waiting to be backported to other branches. Team:Integrations Label for the Integrations team v7.8.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants