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

Add support for once mode; run processors and aggregators during test. #7474

Merged
merged 7 commits into from
Jun 1, 2020

Conversation

danielnelson
Copy link
Contributor

This PR contains several changes over #7408:

  • Resolved conflicts on feat: RunOnce / Serverless Telegraf #7408 due to ticker changes.
  • Fixed buffering issues by running full pipeline.
  • Added special handling for cpu/mongodb/procstat to mirror --test behavior.
  • Allowed using --once with the --test-wait flag.
  • Renamed cli option to --once (let me know if you think this is to terse)
  • Updated usage message
  • Combined with --test, which now runs processors and aggregators at last.

There are several behavior changes compared to current behavior:

  • --test now runs all inputs, previously stopped on first failure. Exit code is based on agent internal/selfstats.
  • --test mode now runs all inputs concurrently, this means that output lines will generally be mixed.

closes #6190
closes #7408
closes #7415

Required for all PRs:

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

@danielnelson danielnelson added the feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin label May 6, 2020
@danielnelson danielnelson added this to the 1.15.0 milestone May 6, 2020
Copy link
Contributor

@ssoroka ssoroka left a comment

Choose a reason for hiding this comment

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

Much anticipated PR. ❤️
Couple suggestions

agent/agent.go Outdated

ticker := NewUnalignedTicker(interval, jitter)
defer ticker.Stop()
<-ticker.Elapsed()
Copy link
Contributor

Choose a reason for hiding this comment

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

don't we want to collect immediately for the -test and -once command line arguments?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed

agent/agent.go Outdated
Comment on lines 286 to 295
switch input.Config.Name {
case "cpu", "mongodb", "procstat":
nulAcc := NewAccumulator(input, nul)
nulAcc.SetPrecision(a.Precision())
if err := input.Input.Gather(nulAcc); err != nil {
nulAcc.AddError(err)
}

time.Sleep(500 * time.Millisecond)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

lost the comment here, but it'd still be helpful. Very wtf inducing. Or.. This might go well in a separate function, then you could name it gatherOnceAndDiscard or something.

agent/agent.go Outdated
}
wg.Wait()

internal.SleepContext(ctx, wait)
Copy link
Contributor

Choose a reason for hiding this comment

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

do -test and -once really need to sleep?

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 for the --test-wait cli option, which I am supporting here in --once mode as well. This option can be used to let service inputs run for a bit, allowing you to test plugins like influxdb_listener.

agent/agent.go Outdated
acc.AddError(err)
hasErrors = true
}
var wg sync.WaitGroup
Copy link
Contributor

Choose a reason for hiding this comment

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

we have variable shadowing here. It'd be more clear to give this a different name.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

agent/agent.go Outdated
}
}
wg.Wait()
Copy link
Contributor

Choose a reason for hiding this comment

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

just clarifying that this is a wait on the internal wg, not the external wg.

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 created a function for running the inputs in test mode, but yeah it was.

@danielnelson danielnelson requested a review from ssoroka May 21, 2020 00:11
@jacquesh
Copy link
Contributor

Just dropping in to say that I was directed here from #7569 (I hadn't seen this PR, sorry!):
After some quick tests in almost-exactly the setup I want it for suggest that this is working as I would expect in my use-case 👍

Copy link
Contributor

@reimda reimda left a comment

Choose a reason for hiding this comment

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

The duplication between Run() and test() kind of bothers me but I don't think it would be worth it to try to combine them. Also the way delta metrics are hard coded by name ("cpu", "mongodb", "procstat") is ugly but it's not new code.

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.

telegraf --test fails with bind: address already in use --test flag doesn't run processors or aggregators
4 participants