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

feat(win_perf_counters): Added support for reading raw values #6501

Merged
merged 1 commit into from
Feb 10, 2022
Merged

feat(win_perf_counters): Added support for reading raw values #6501

merged 1 commit into from
Feb 10, 2022

Conversation

vlastahajek
Copy link
Contributor

Description

Added support for reading raw values of performance counters.
Fixes #5198

Required for all PRs:

  • Signed CLA.
  • Associated README.md updated.
  • Has appropriate unit tests.

glinton
glinton reviewed Oct 9, 2019
View reviewed changes
plugins/inputs/win_perf_counters/README.md Show resolved Hide resolved
plugins/inputs/win_perf_counters/win_perf_counters.go Outdated
if collectFields[instance] == nil {
collectFields[instance] = make(map[string]interface{})
}
collectFields[instance][sanitizedChars.Replace(metric.counter)] = float32(value)
collectFields[instance][metric.counter] = value
Copy link
Contributor

Choose a reason for hiding this comment

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

Should there be a type switch here to preserve the value types that were cast to a float32?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The original conversion to float32 was there before I got this area and it's worthless. PDH API returns float values in the C double (64bit) type, so it does not make sense to convert them to float32.

The true type of counter is returned from specific function for request format reading here is only assigned to storage and later added to accumulator where fields are also type independent.

@gregvolk
Copy link
Contributor

I've been testing this branch on multiple windows systems and it works well. As expected I had to apply some perSecond and scaling functions on the TSDB / Grafana end to compute the rates of change over time on the raw metric data. The metric data looks accurate and sane after the functions are applied.

Thanks for implementing this Vlasta!

Can someone merge this PR so that this makes it into a release?

@vlastahajek
Copy link
Contributor Author

Thanks for testing, @gregvolk ! There is just a discussion about configuration design that needs to be concluded.

@sjwang90 sjwang90 added area/windows Related to windows plugins (win_eventlog, win_perf_counters, win_services) feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin labels Nov 24, 2020
@sjwang90 sjwang90 added this to the Planned milestone Dec 9, 2020
@sjwang90 sjwang90 removed this from the Planned milestone Jan 29, 2021
@gregvolk
Copy link
Contributor

@danielnelson can this PR be accepted and merged? I tested @vlastahajek's contrib a while back and it works great. I'm looking to roll out the latest version of Telegraf to all my servers and it would be nice to have this feature. Thanks.

@sspaink
Copy link
Contributor

sspaink commented Jan 27, 2022 •
edited
Loading

@vlastahajek are you still interested in continuing working on this PR? If you could you re-base it then the latest CI can run.
@gregvolk thank you for testing it, we can't merge this PR quite yet not until it has been re-based and all the tests are passing. Looking at the old discussions I think updating the config to use the suggested UseValues = "raw|formatted" is also a good idea.

@vlastahajek
Copy link
Contributor Author

@sspaink, I'm working on rebasing this. There is a small problem with the last change by #10101, which broke a test. I'm getting back into the picture to see what is wrong.

@sspaink sspaink changed the title win_perf_counters: Added support for reading raw values feat(win_perf_counters): Added support for reading raw values Jan 28, 2022
@vlastahajek
Copy link
Contributor Author

It should be ready.

@telegraf-tiger
Copy link
Contributor

☺️ This pull request doesn't significantly change the Telegraf binary size (less than 1%)

📦 Looks like new artifacts were built from this PR.

Expand this list to get them here ! 🐯

Artifact URLs

DEB RPM TAR GZ ZIP
amd64.deb aarch64.rpm darwin_amd64.tar.gz windows_amd64.zip
arm64.deb armel.rpm darwin_arm64.tar.gz windows_i386.zip
armel.deb armv6hl.rpm freebsd_amd64.tar.gz
armhf.deb i386.rpm freebsd_armv7.tar.gz
i386.deb ppc64le.rpm freebsd_i386.tar.gz
mips.deb riscv64.rpm linux_amd64.tar.gz
mipsel.deb s390x.rpm linux_arm64.tar.gz
ppc64el.deb x86_64.rpm linux_armel.tar.gz
riscv64.deb linux_armhf.tar.gz
s390x.deb linux_i386.tar.gz
linux_mips.tar.gz
linux_mipsel.tar.gz
linux_ppc64le.tar.gz
linux_riscv64.tar.gz
linux_s390x.tar.gz
static_linux_amd64.tar.gz

@sspaink sspaink added the ready for final review This pull request has been reviewed and/or tested by multiple users and is ready for a final review. label Feb 1, 2022
@MyaLongmire MyaLongmire merged commit 3ef1c73 into influxdata:master Feb 10, 2022
pteich added a commit to pteich/telegraf that referenced this pull request Feb 13, 2022
@pteich
Merge branch 'master' into feature/opensearch-support
0afaf35 
* master: (117 commits)
  fix: bump github.com/nats-io/nats-server/v2 from 2.6.5 to 2.7.2 (influxdata#10638)
  chore: add -race flag to go tests (influxdata#10629)
  feat: update etc/telegraf.conf and etc/telegraf_windows.conf (influxdata#10631)
  fix: license doc outdated causing CI failure (influxdata#10630)
  fix: bump k8s.io/client-go from 0.22.2 to 0.23.3 (influxdata#10589)
  feat: Implemented support for reading raw values, added tests and doc (influxdata#6501)
  fix: Improve parser tests by using go-cmp/cmp (influxdata#10497)
  feat(mongodb): add FsTotalSize and FsUsedSize informations (influxdata#10625)
  docs: update quay docs for auth (influxdata#10612)
  chore: allow downgrade of go version in windows script (influxdata#10614)
  chore: update CI go to 1.17.6 (influxdata#10611)
  feat: update etc/telegraf.conf and etc/telegraf_windows.conf (influxdata#10600)
  fix(inputs.opcua): add more data to error log (influxdata#10465)
  fix: bump github.com/aws/aws-sdk-go-v2/service/kinesis from 1.6.0 to 1.13.0 (influxdata#10601)
  refactor: use early return pattern (influxdata#10591)
  fix: bump github.com/benbjohnson/clock from 1.1.0 to 1.3.0 (influxdata#10588)
  feat: add dynamic tagging to gnmi plugin (influxdata#7484)
  fix: bump github.com/Azure/azure-kusto-go from 0.5.0 to 0.5.2 (influxdata#10598)
  feat: update etc/telegraf.conf and etc/telegraf_windows.conf (influxdata#10584)
  fix(parsers.json_v2): allow optional paths and handle wrong paths correctly (influxdata#10468)
  ...

# Conflicts:
#	plugins/outputs/elasticsearch/elasticsearch.go
#	plugins/outputs/elasticsearch/elasticsearch_test.go
phemmer added a commit to phemmer/telegraf that referenced this pull request Feb 18, 2022
@phemmer
Merge remote-tracking branch 'origin/master' into postgres
e87744d 
* origin/master: (211 commits)
  feat: update configs (influxdata#10676)
  feat[elastic output]: add elastic pipeline flags (influxdata#10505)
  Update changelog
  fix: ensure folders do not get loaded more than once (influxdata#10551)
  docs: update VMWare doc links (influxdata#10663)
  fix: prometheusremotewrite wrong timestamp unit (influxdata#10547)
  feat: update configs (influxdata#10662)
  fix: add graylog toml tags (influxdata#10660)
  feat: add socks5 proxy support for kafka output plugin (influxdata#8192)
  docs: override reported OpenSearch version (influxdata#10586)
  feat: update etc/telegraf.conf and etc/telegraf_windows.conf (influxdata#10659)
  fix: bump all go.opentelemetry.io dependencies (influxdata#10647)
  feat: collection offset implementation (influxdata#10545)
  chore: update go to 1.17.7 (influxdata#10658)
  fix: check for nil client before closing in amqp (influxdata#10635)
  fix: timestamp change during execution of json_v2 parser. (influxdata#10657)
  fix: bump github.com/signalfx/golib/v3 from 3.3.38 to 3.3.43 (influxdata#10652)
  fix: bump github.com/aliyun/alibaba-cloud-sdk-go (influxdata#10653)
  fix: incorrect handling of json_v2 timestamp_path (influxdata#10618)
  feat: gather additional stats from memcached (influxdata#10641)
  feat: update etc/telegraf.conf and etc/telegraf_windows.conf (influxdata#10649)
  fix: Revert deprecation of http_listener_v2 (influxdata#10648)
  fix: bump github.com/denisenkom/go-mssqldb from 0.10.0 to 0.12.0 (influxdata#10503)
  fix: bump github.com/gopcua/opcua from 0.2.3 to 0.3.1 (influxdata#10626)
  fix: use current time as ecs timestamp (influxdata#10636)
  fix: bump github.com/nats-io/nats-server/v2 from 2.6.5 to 2.7.2 (influxdata#10638)
  chore: add -race flag to go tests (influxdata#10629)
  feat: update etc/telegraf.conf and etc/telegraf_windows.conf (influxdata#10631)
  fix: license doc outdated causing CI failure (influxdata#10630)
  fix: bump k8s.io/client-go from 0.22.2 to 0.23.3 (influxdata#10589)
  feat: Implemented support for reading raw values, added tests and doc (influxdata#6501)
  fix: Improve parser tests by using go-cmp/cmp (influxdata#10497)
  feat(mongodb): add FsTotalSize and FsUsedSize informations (influxdata#10625)
  docs: update quay docs for auth (influxdata#10612)
  chore: allow downgrade of go version in windows script (influxdata#10614)
  chore: update CI go to 1.17.6 (influxdata#10611)
  feat: update etc/telegraf.conf and etc/telegraf_windows.conf (influxdata#10600)
  fix(inputs.opcua): add more data to error log (influxdata#10465)
  fix: bump github.com/aws/aws-sdk-go-v2/service/kinesis from 1.6.0 to 1.13.0 (influxdata#10601)
  refactor: use early return pattern (influxdata#10591)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@glinton glinton glinton left review comments

@MyaLongmire MyaLongmire MyaLongmire approved these changes

@powersj powersj powersj approved these changes

Assignees
No one assigned
Labels
area/windows Related to windows plugins (win_eventlog, win_perf_counters, win_services) feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin ready for final review This pull request has been reviewed and/or tested by multiple users and is ready for a final review.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Support the Win32_PerfRawData class instead of Win32_PerfFormattedData
7 participants
@vlastahajek @gregvolk @sspaink @glinton @powersj @MyaLongmire @sjwang90