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

Fix empty field error in the iis/application pool metricset #19537

Merged
merged 11 commits into from
Jul 14, 2020

Conversation

narph
Copy link
Contributor

@narph narph commented Jul 1, 2020

What does this PR do?

Fixes errors as:

"metricset":{"name":"application_pool","period":10000},"service":{"type":"iis"}}, Private:interface {}(nil), TimeSeries:true}, Flags:0x0, Cache:publisher.EventCache{m:common.MapStr(nil)}} (status=400): {"type":"mapper_parsing_exception","reason":"failed to parse","caused_by":{"type":"illegal_argument_exception","reason":"field name cannot be an empty string"}}

also improves refresh functionality

Why is it important?

Errors

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.

How to test this PR locally

Run the IIS module wit the application pool metricset enabled

@botelastic botelastic bot added the needs_team Indicates that the issue/PR needs a Team:* label label Jul 1, 2020
@narph narph self-assigned this Jul 1, 2020
@narph narph added the Team:Integrations Label for the Integrations team label Jul 1, 2020
@elasticmachine
Copy link
Collaborator

Pinging @elastic/integrations (Team:Integrations)

@botelastic botelastic bot removed the needs_team Indicates that the issue/PR needs a Team:* label label Jul 1, 2020
@elasticmachine
Copy link
Collaborator

elasticmachine commented Jul 1, 2020

💚 Build Succeeded

Pipeline View Test View Changes Artifacts preview

Expand to view the summary

Build stats

  • Build Cause: [Pull request #19537 updated]

  • Start Time: 2020-07-14T09:18:42.942+0000

  • Duration: 41 min 51 sec

Test stats 🧪

Test Results
Failed 0
Passed 745
Skipped 72
Total 817

Steps errors

Expand to view the steps failures

  • Name: Report to Codecov
    • Description: curl -sSLo codecov https://codecov.io/bash for i in auditbeat filebeat heartbeat libbeat metricbeat packetbeat winlogbeat journalbeat do FILE="${i}/build/coverage/full.cov" if [ -f "${FILE}" ]; then bash codecov -f "${FILE}" fi done

    • Duration: 1 min 27 sec

    • Start Time: 2020-07-14T09:55:42.040+0000

    • log

@narph narph added the needs_backport PR is waiting to be backported to other branches. label Jul 1, 2020
return nil, errors.Wrapf(err, `failed to add counter (query="%v")`, v)
}
r.workerProcesses[v] = key
}
Copy link
Member

Choose a reason for hiding this comment

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

Code added here is mostly the same as in refreshCounterPaths(), could it be reused?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

refactored the code so the function is reused

// refresh performance counter list
// Some counters, such as rate counters, require two counter values in order to compute a displayable value. In this case we must call PdhCollectQueryData twice before calling PdhGetFormattedCounterValue.
// For more information, see Collecting Performance Data (https://docs.microsoft.com/en-us/windows/desktop/PerfCtrs/collecting-performance-data).
// A flag is set if the second call has been executed else refresh will fail (reader.executed)
Copy link
Member

Choose a reason for hiding this comment

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

Code added to newReader seems to be the same as a refresh, and it is run before reader.executed, could it also fail?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we try to run one refresh at a time, running them one after another seems to yield exceptions

}
apps, err := getApplicationPools(config.Names)
if err != nil {
return r, errors.Wrap(err, "failed retrieving running worker processes")
Copy link
Member

Choose a reason for hiding this comment

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

If something fails here, should we close query to avoid leaks?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

indeed, added the call

@narph narph requested a review from jsoriano July 14, 2020 09:18
@@ -81,6 +84,7 @@ func (m *MetricSet) Fetch(report mb.ReporterV2) error {
break
}
}

Copy link
Member

Choose a reason for hiding this comment

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

nit: not related?

@@ -9,40 +9,41 @@ package application_pool
import (
"strings"

"github.com/elastic/beats/v7/libbeat/common"
"github.com/elastic/beats/v7/metricbeat/helper/windows/pdh"
Copy link
Member

Choose a reason for hiding this comment

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

nit: please fix import's order

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ran fmt so that should be the order

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 think that mage fmt touches the order, I just made a small local test and it seems that doesn't. You can verify that by putting them in order and running again fmt.
Not sure how this change occurred and preserving order is not crucial but it's a more convention we follow.

@narph narph requested a review from ChrsMark July 14, 2020 11:07
Copy link
Member

@ChrsMark ChrsMark left a comment

Choose a reason for hiding this comment

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

looks good! It would be nice to address #19537 (comment)

@narph narph merged commit 93e43e7 into elastic:master Jul 14, 2020
@narph narph deleted the app-pool-fix branch July 14, 2020 12:23
narph added a commit to narph/beats that referenced this pull request Jul 14, 2020
…19537)

* fix

* update test

* changelog

* change

* refactor

* close

(cherry picked from commit 93e43e7)
narph added a commit to narph/beats that referenced this pull request Jul 14, 2020
…19537)

* fix

* update test

* changelog

* change

* refactor

* close

(cherry picked from commit 93e43e7)
narph added a commit that referenced this pull request Jul 14, 2020
…19886)

* fix

* update test

* changelog

* change

* refactor

* close

(cherry picked from commit 93e43e7)
narph added a commit that referenced this pull request Jul 14, 2020
…on pool metricset (#19887)

* Fix empty field error in the iis/application pool metricset (#19537)

* fix

* update test

* changelog

* change

* refactor

* close

(cherry picked from commit 93e43e7)

* update changelog
melchiormoulin pushed a commit to melchiormoulin/beats that referenced this pull request Oct 14, 2020
…19537)

* fix

* update test

* changelog

* change

* refactor

* close
leweafan pushed a commit to leweafan/beats that referenced this pull request Apr 28, 2023
…plication pool metricset (elastic#19887)

* Fix empty field error in the iis/application pool metricset (elastic#19537)

* fix

* update test

* changelog

* change

* refactor

* close

(cherry picked from commit 134324e)

* update changelog
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs_backport PR is waiting to be backported to other branches. Team:Integrations Label for the Integrations team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants