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

[receiver/windowsperfcountersreceiver] Update Windows performance counter to be able to better define metrics. #8259

Closed
Mrod1598 opened this issue Mar 4, 2022 · 7 comments
Labels
enhancement New feature or request os:windows

Comments

@Mrod1598
Copy link
Contributor

Mrod1598 commented Mar 4, 2022

Is your feature request related to a problem? Please describe.
The windows performance counter receiver doesn't give much control over the metrics that are defined to be pulled from the defined counters.

Describe the solution you'd like
Update the existing receiver to give more in depth configuration.

an example of what a new configuration could be:

windowsperfcounters/other:
  collection_interval: 1m
  metrics:
    - metric_name: cpu.time
      description: the amount of time
      unit: "%"
      metric_type: gauge 
  perfcounters:
    - object: "Processor"
      instances: [1, 2]
      counters: 
        - counter_name: "% Idle Time"
          metric_name: cpu.time
          attributes:
            - time: idle
        - counter_name: "% Active Time"
          metric_name: cpu.time
          attributes:
            - time: active

Describe alternatives you've considered
There is the ability to apply transforms to the data to get it in the proper format but it seems better to be able to do it up front rather than establishing all metrics as double gauges and then having to transform them.

@jpkrohling
Copy link
Member

cc @dashpole

@dashpole dashpole added enhancement New feature or request os:windows labels Mar 7, 2022
@djaglowski
Copy link
Member

I think this points out a necessary change. It's not acceptable in the longer term that this receiver would produce metrics in such a generic and often inaccurate way.

The proposal overall makes sense to me. A few observations / points of feedback:

  1. Overall, this appears to be an additive change. metrics is new, and the individual counters have a couple new fields. I'm not sure it's strictly necessary to make this change backwards compatible, but it seems easily doable here.
  2. The metrics key is used in other scrapers to represent mdatagen's MetricsSettings. This receiver may or may not ever support that capability, but user expectations may warrant that this key is something else like metric_metadata or just metadata.
  3. Nit: attributes should be a map.

The major open question to me is how best to not reinvent the wheel when it comes to defining metrics in yaml. It would be great if we can reuse mdatagen's yaml structure here. It is designed for the same purpose really, so seems suitable. It's not currently exported and I assume we'd want to extract it out of the cmd package.

@dmitryax, do you think mdatagen/v2 has become stable enough that we could extract the metric metadata representation and embed it in a receiver config? Would value any other thoughts you have on this approach or something similar.

Incorporating what I've proposed above, the updated config would look like:

windowsperfcounters/other:
  collection_interval: 1m
  metadata:
    cpu.time:
      enabled: true
      description: The foo of the bar.
      unit: %
      gauge:
        value_type: int
      attributes: [ time ]
  perfcounters:
    - object: "Processor"
      instances: [1, 2]
      counters: 
        - counter_name: "% Idle Time"
          metric_name: cpu.time
          attributes:
            time: idle
        - counter_name: "% Active Time"
          metric_name: cpu.time
          attributes:
            time: active

@dmitryax
Copy link
Member

dmitryax commented Mar 9, 2022

I'm not familiar with this receiver yet. Is there any issues to migrate it to metrics builder? I believe doing that already resolves a part of this issue. And to cover remaining parts of this issue, we can probably extend metrics builder functionality

@djaglowski
Copy link
Member

I don't think this receiver could use metrics builder because it does not collect predefined metrics. It is up to the user to specify which performance counters should be collected. Correspondingly, this change suggests that the user should be able to articulate the characteristics of the metric into which the counter is converted.

@dmitryax
Copy link
Member

dmitryax commented Mar 9, 2022

It is up to the user to specify which performance counters should be collected.

But isn't the set of performance counters limited? Potentially we can define all of them as optional metrics, right?

Correspondingly, this change suggests that the user should be able to articulate the characteristics of the metric into which the counter is converted.

I thinking if this is something that can be covered by additional features of the metrics builder

@djaglowski
Copy link
Member

But isn't the set of performance counters limited? Potentially we can define all of them as optional metrics, right?

I'm not a Windows expert but my understanding is that Windows Performance Counters are basically a metrics framework, akin to MBeans.

There are many standard counters for Windows itself and many more for common applications like SQL Server. However, I'm not sure it's reasonable to try to describe all of them (though I certainly could be wrong).

More importantly, I think Windows application developers can define and report their own counters.

@dmitryax
Copy link
Member

dmitryax commented Mar 9, 2022

Ok makes sense. In that case we can apply the approach that you suggested. I'd say metadata API is stable enough for this. There are still may be small changes and additions to it tho

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request os:windows
Projects
None yet
Development

No branches or pull requests

5 participants