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

feat(plugins.nvidia_smi): Implement Probe() #16305

Draft
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

LandonTClipp
Copy link
Contributor

@LandonTClipp LandonTClipp commented Dec 13, 2024

Summary

This PR implements tsd-009 at various levels:

  1. Adds a Probe method to the nvidia_smi plugin.
  2. Adds a Probe method to the RunningInput model that will selectively call the Probe method of the plugin if available and configured as such in startup_error_behavior.
  3. Modifies the Start method of RunningInput such that if the plugin returns an error from Start, we treat it the same as startup_error_behavior = ignore.
  4. Adds logic in the Agent to call RunningInput.Probe() after the plugin has started. If Probe() returns a non-nil error, the agent will unconditionally skip the plugin.

Checklist

  • No AI generated code was used in this PR

Related issues

@LandonTClipp LandonTClipp changed the title WIP: feat(plugins.nvidia_smi): Implement Probe() feat(plugins.nvidia_smi): Implement Probe() Dec 13, 2024
@telegraf-tiger telegraf-tiger bot added the feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin label Dec 13, 2024
@LandonTClipp LandonTClipp marked this pull request as ready for review December 13, 2024 19:29
@telegraf-tiger
Copy link
Contributor

Copy link
Member

@srebhan srebhan left a comment

Choose a reason for hiding this comment

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

Thank you for the PR @LandonTClipp! I do have two comments in the code and want to ask you to split the PR into

  1. adding the probing framework
  2. adding probing support to NVidia SMI

Comment on lines +173 to +182
func (r *RunningInput) Probe() error {
p, ok := r.Input.(telegraf.ProbePlugin)
if !ok || r.Config.StartupErrorBehavior != "probe" {
r.log.Debug("Not probing plugin")
return nil
}
r.log.Debug("Probing plugin")
return p.Probe()
}

Copy link
Member

Choose a reason for hiding this comment

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

Please remove the debug messages here as they don't add value for the user!

Suggested change
func (r *RunningInput) Probe() error {
p, ok := r.Input.(telegraf.ProbePlugin)
if !ok || r.Config.StartupErrorBehavior != "probe" {
r.log.Debug("Not probing plugin")
return nil
}
r.log.Debug("Probing plugin")
return p.Probe()
}
func (r *RunningInput) Probe() error {
p, ok := r.Input.(telegraf.ProbePlugin)
if !ok || r.Config.StartupErrorBehavior != "probe" {
return nil
}
return p.Probe()
}

Comment on lines +261 to +269

type mockProbingInput struct {
probeReturn error
}

func (m *mockProbingInput) SampleConfig() string {
return ""
}

Copy link
Member

Choose a reason for hiding this comment

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

I think you don't need the test here as the testing in running_input_test.go already covers the case...

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 thought it would be good to test that the agent is actually calling Probe but if you prefer not to have this test, I'll remove it.

@srebhan srebhan self-assigned this Dec 17, 2024
@LandonTClipp
Copy link
Contributor Author

Will do!

@LandonTClipp LandonTClipp marked this pull request as draft December 19, 2024 17:42
@LandonTClipp
Copy link
Contributor Author

Converting to draft until #16333 is merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

2 participants