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

Perfmon wildcard queries #4502

Merged
merged 13 commits into from
Jul 11, 2017
Merged

Conversation

martinscholz83
Copy link
Contributor

Add the possibility to perform wildcard queries for instances. See #3828

@karmi
Copy link

karmi commented Jun 14, 2017

Hi @maddin2016, we have found your signature in our records, but it seems like you have signed with a different e-mail than the one used in yout Git commit. Can you please add both of these e-mails into your Github profile (they can be hidden), so we can match your e-mails to your Github profile?

@elasticmachine
Copy link
Collaborator

Can one of the admins verify this patch?

@elasticmachine
Copy link
Collaborator

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically on build-eu-00. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

@martinscholz83
Copy link
Contributor Author

martinscholz83 commented Jun 14, 2017

Hey @andrewkroh, @ruflin. Sorry for the delay. I added the function to perform wildcard queries. I just check if the path contains (*)\. Because a wildcard query return all instances, i decided to put them together each as an object. For example

diskio: {
    write: {
        bytes: {
            "C:": 2500.0000,
            "D": 1200.0000
            }
        }
    }

Maybe you have any other suggestions.

@martinscholz83 martinscholz83 changed the title Perfmon wildcards Perfmon wildcard queries Jun 14, 2017
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.

Thanks for continuing to improve the perfmon metricset!

There is a binary file (metricbeat/debug) in the PR that I think was added accidentally.

}
defer handle.query.Close()

values, err := handle.Read()
Copy link
Member

Choose a reason for hiding this comment

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

Why is the error not checked? If you intentially are going to ignore an error then I recommend writing the assignment as values, _ := handle.Read() to be clear that it was intentional.

println(k)
}

if m, n := values["processor"].(common.MapStr); n {
Copy link
Member

Choose a reason for hiding this comment

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

If processor or processor.time or processors.time.pct are not found, should this be a test failure? I'm pretty sure this block could be simplified to:

pcts, err := values.GetValue("processors.time.pct`)
if err != nil {
  t.Fatal(err)
}
assert.NotEmpty(t, pcts)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

VS Code Intellisense wasn't working 😄 That's what i've searched for.

}

for k, _ := range values {
println(k)
Copy link
Member

Choose a reason for hiding this comment

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

Use t.Log() so that the -v flag is honored for tests.

@@ -77,6 +80,26 @@ func PdhGetFormattedCounterValue(counter PdhCounterHandle, format PdhCounterForm
return counterType, &value, nil
}

func PdhGetFormattedCounterArray(counter PdhCounterHandle, format PdhCounterFormat) (int, *[]PdhCounterValueItem, error) {
Copy link
Member

Choose a reason for hiding this comment

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

Why not return []PdhCounterValueItem instead of a *[]PdhCounterValueItem?

Copy link
Member

Choose a reason for hiding this comment

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

Also I don't think it should return the int when that can be obtained using len() on the returned slice.

Copy link
Member

Choose a reason for hiding this comment

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

After looking at the PdhCounterValueItem and how it's used, I think it would be best to return a different struct that has the a Name field that is a string. This way the users of the method don't need to follow the pointer and convert the UTF16 bytes to UTF8.

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 returned slice is larger. So we need the bufferCount value to iterate over the slice to get the items which has a value.

)

var (
errERROR_IO_PENDING error = syscall.Errno(errnoERROR_IO_PENDING)
Copy link
Member

Choose a reason for hiding this comment

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

I don't see where this is used?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is coming from go generate. I think its since go 1.8.x.

@andrewkroh
Copy link
Member

andrewkroh commented Jun 14, 2017

Because a wildcard query return all instances, i decided to put them together each as an object.

Because the are separate instances of the same counter it seems like each instance should be its own event so that they can easily be compared to each other like apples to apples. For example you might want to create a line graph showing diskio for C, D, and E all at once. Having a data structure similar to what we have now with the non-wildcard would make this easier. But a field would need to be added specifying the instance name (like C, D, or E).

{
  "windows":{
    "perfmon":{
      "disk":{
        "name":"A",
        "bytes":{
          "read":{
            "total":0
          }
        }
      }
    }
  }
}

So maybe in the config you have something like:

instance_label: disk.name
measurement_label: disk.bytes.read.total

@martinscholz83
Copy link
Contributor Author

martinscholz83 commented Jun 21, 2017

@andrewkroh, thanks for your comments. Based on this i have changed the logic and fixed the test. To do is to separate the instances.

@martinscholz83
Copy link
Contributor Author

martinscholz83 commented Jun 23, 2017

Maybe we should make this config standard. Lets take the following config

perfmon.counters:
    - instance_label: processor.name
      measurement_label: processor.time.total
      query: '\Processor Information(_Total)\% Processor Time'

This would produce the following output

{
  "windows":{
    "perfmon":{
      "processor":{
        "name":"_Total",
        "time":{
            "total":0          
        }
      }
    }
  }
}

So here perfmon produce only one value _Total. Unfortunately the api doesn't return the instance name for a single instance. So we have to extract it from the query string. For wildcard queries we can get the instance name from the returned array.

What do you think about?

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.

Yes, I think having a single way to configure all the counters is more clear to the user.

break
}
}
runes := utf16.Decode(a[:size:size])
Copy link
Member

Choose a reason for hiding this comment

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

That second size should be unnecessary as that is the capacity, right?


for i = 0; i < bufferCount; i++ {
value := CounterValueItem{}
a := (*[1<<30 - 1]uint16)(unsafe.Pointer(pdhValues[i].SzName))
Copy link
Member

Choose a reason for hiding this comment

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

We should try to find a more "safe" way to do this in the future. Using the unsafe pointer to cast an arbitrary size array worries me. But that's for another PR another time. :)

@@ -80,24 +85,43 @@ func PdhGetFormattedCounterValue(counter PdhCounterHandle, format PdhCounterForm
return counterType, &value, nil
}

func PdhGetFormattedCounterArray(counter PdhCounterHandle, format PdhCounterFormat) (int, *[]PdhCounterValueItem, error) {
func PdhGetFormattedCounterArray(counter PdhCounterHandle, format PdhCounterFormat) ([]CounterValueItem, error) {
Copy link
Member

Choose a reason for hiding this comment

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

That looks better.

@martinscholz83
Copy link
Contributor Author

@andrewkroh, tests are passing on my local machine. Can you please run the tests again. Thanks!

@andrewkroh
Copy link
Member

jenkins, test it

@andrewkroh
Copy link
Member

Ok, I triggered a rebuild of AppVeyor and Jenkins.

@martinscholz83
Copy link
Contributor Author

What is this TestData test which is failing??

@andrewkroh
Copy link
Member

andrewkroh commented Jul 6, 2017

I'm seeing it fail before getting to TestData.

Building metricbeat
# github.com/elastic/beats/metricbeat/module/system/process
module\system\process\helper.go:398: undefined: sort.Slice
module\system\process\helper.go:405: undefined: sort.Slice

But TestData is used to generate the _meta/data.json file that contains a sample event that will show up in the docs.

@martinscholz83
Copy link
Contributor Author

🙈 This happens if you work on 3 different machines 😑

@martinscholz83
Copy link
Contributor Author

martinscholz83 commented Jul 7, 2017

Why it doesn't implement EventFetcher interface?
My mistake. Called wrong function f := mbtest.NewEventsFetcher(t, config). Missed the s.

@andrewkroh andrewkroh merged commit 5f959cd into elastic:master Jul 11, 2017
@andrewkroh
Copy link
Member

🎉 Thanks!

andrewkroh added a commit to andrewkroh/beats that referenced this pull request Sep 7, 2017
tsg pushed a commit that referenced this pull request Sep 8, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants