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

Interrupts+spurious #5519

Closed

Conversation

richardelling
Copy link
Contributor

Adds an optional spurious_interrupts measurement for tracking spurious interrupts.

Also, removes metrics for phantom CPUs: those who may be reported in the /proc/softirqs, but might not actually exist and only include zeroes. See also #5451

Required for all PRs:

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

@glinton glinton added the feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin label Mar 4, 2019
plugins/inputs/interrupts/interrupts.go Show resolved Hide resolved
|:---:|:---:|:---:|---|
| count | counter | events | number of times the interrupt has been handled (modulo 100,000) |
| total | counter | events | total number of times the interrupt has been handled |
| unhandled | counter | events | number of times an interrupt was not handled |
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks nice but we need to stick to the previous format style to match https://raw.githubusercontent.com/influxdata/telegraf/master/plugins/inputs/EXAMPLE_README.md

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will do, though I will assert your standard is ugly and takes up too much screen space :-)

@@ -13,65 +14,99 @@ The interrupts plugin gathers metrics about IRQs from `/proc/interrupts` and `/p
## deployments.
# cpu_as_tag = false

## spurious interrupt counters can be collected
# spurious = false
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's just enable this by default, and rely on the metric filtering for those who don't want spurious counts per IRQ.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

defer f.Close()
irqs, err := parseInterrupts(f)
irqs, err := parseInterrupts(f, s.Spurious)
_ = f.Close()
Copy link
Contributor

Choose a reason for hiding this comment

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

The previous defer method is actually preferred, since it will be less likely to be broken in future code updates.

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 problem with the previous method is that the static code checkers complain about the f.close() return code being ignored, which it is. Two choices:

  1. add proper func for defer f.Close() to explicitly ignore the return value
  2. add comment for static checkers to ignore the ignored return value error

I'm fine either way. Do you have a preference?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not our goal to keep static code checkers quiet, outside of the ones required by the integration tests (go vet in particular). So I'd rather just leave it as before, I don't find assigning to _ to be helpful in explaining the code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I'll prefer to f.Close() in the loop. If defer is in a loop, we trigger possible leak detection because the defer's closure is the function, not the loop. In this particular location inside Gather, we loop twice, not a real problem. In parseInterrupts we open one file per irq, this can easily grow into the hundreds of open file descriptors, so it is better to close in the loop.

Copy link
Contributor

Choose a reason for hiding this comment

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

Good point, this is possibly an indication that we should extract a function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, I'm not particularly happy with the overall design, lemme try a more detailed refactor, including some improved testing

@srebhan
Copy link
Member

srebhan commented Mar 9, 2021

@richardelling any update on this? The code looks quite nice already and it would be a pitty if it bit-rots like this.

@srebhan srebhan self-assigned this Mar 9, 2021
@srebhan
Copy link
Member

srebhan commented Apr 28, 2021

@richardelling do you see a chance to work on this PR?

@srebhan srebhan added the waiting for response waiting for response from contributor label Apr 28, 2021
@srebhan
Copy link
Member

srebhan commented Jun 1, 2021

Anyone interested in taking over this PR?

@srebhan srebhan added help wanted Request for community participation, code, contribution and removed waiting for response waiting for response from contributor labels Jun 1, 2021
@richardelling
Copy link
Contributor Author

@srebhan thanks for the ping, I'll not have time until next month at the earliest. I

@srebhan srebhan removed the help wanted Request for community participation, code, contribution label Jun 2, 2021
@srebhan
Copy link
Member

srebhan commented Jun 2, 2021

@richardelling thank you for letting me know. Please drop me a note once you get to it (or decide to drop this PR). :-)

@richardelling
Copy link
Contributor Author

@srebhan closing this in favor of a newer PR #9496

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/system feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants