-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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/iis] Add IIS Metrics Receiver #8832
Conversation
@djaglowski ok this is ready for a look when you get a second. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall this is looking good to me.
Is it possible to add integration tests? It seems likely these counters are available on the default windows image used by github actions.
here's a list of the tech that comes installed on github actions boxes, doesn't look like IIS is installed |
To be honest I don't think using
I think we should the following:
@Mrod1598 @djaglowski WDYT? |
@dmitryax I think your proposal makes sense. I think there are circumstances where wrapping or composing other components is a reasonable approach, but I don't think it works well in this case. |
@djaglowski This is ready for another review when you're ready. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks really good. Just a few nits & need to use scrapererror
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also need a codeowners entry for the component. Please list yourself, as well as me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Description:
Add IIS Metrics receiver using windows performance counter as a base.
Link to tracking Issue:
#8370
Testing:
Added config testing, factory tests, and receiver tests that ensure the config is built correctly.
Documentation:
Added doc explaining use of receiver.