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.

Sign up for GitHub

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

Chore/collectd install prompt #275

Merged
merged 3 commits into from
Oct 14, 2021
+1 −1
Merged

Chore/collectd install prompt #275

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0275516
chore: add note in prompt for collectd install
GlennChia Sep 29, 2021
2c3851f
chore: add other options for configuring CollectD
GlennChia Sep 29, 2021
8876de5
chore: concise CollectD install warning message
GlennChia Oct 9, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
chore: add other options for configuring CollectD
@GlennChia
GlennChia committed Sep 29, 2021

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
GPG key ID: 4AEE18F83AFDEB23
Expired
Verified
Learn about vigilant mode
commit 2c3851f7aa367b81c23a62499d1561944bdeeeeb
2 changes: 1 addition & 1 deletion tool/processors/collectd/collectd.go
View file
Original file line number Diff line number Diff line change
@@ -21,7 +21,7 @@ func (p *processor) Process(ctx *runtime.Context, config *data.Config) {
if ctx.OsParameter == util.OsTypeWindows {
return
}
yes := util.Yes("Do you want to monitor metrics from CollectD? Note that enabling this requires the collectd software to be installed on your server")
yes := util.Yes("Do you want to monitor metrics from CollectD? Note that depending on the system, some require the CollectD software to be installed on your server (e.g. Amazon Linux 2) or the /usr/share/collectd/types.db file to be created.")
Copy link
Contributor

Choose a reason for hiding this comment

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

Since the CloudWatch Agent could fail to start, I'd recommend making the the "Note" into a flashy "WARNING" which is more likely to be seen by users:

yes := util.Yes("Do you want to monitor metrics from CollectD? WARNING: CollectD must be installed or the Agent will fail to start")

Another idea would be to modify the configuration validator to output a more helpful error message:

ERROR: collectd is configured, but is not installed. Please install collectd or change the config.

I'd like input from others on the preferred solution.

Copy link
Contributor

Choose a reason for hiding this comment

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

It would be important not to break existing user workflows such as causing the config generator to fail if collectd is not installed properly. Someone could config the agent ahead of installing collectd.

What's the current agent behavior if collectd is configured but not installed?

Copy link
Contributor

Choose a reason for hiding this comment

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

The Agent fails to start:
awsdocs/amazon-cloudwatch-user-guide#54

Copy link
Contributor

@SaxyPandaBear SaxyPandaBear Oct 6, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I agree with @adam-mateen on this that it should be made more obvious that there will be an issue if collectd is not installed prior to starting the CW agent.

  1. This doesn't change any behavior - it just makes it clearer to the user that the agent will fail to start if collectd isn't installed
  2. The one thing I'd be a little hesitant on is the length of the input prompt string now, but that's mostly a nitpick. For someone working on a smaller terminal, the formatting might get a little messed up with such a long string. That being said, if there's another prompt in the repo that is also very long, then I have no qualms with this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi all, thanks for the review, I think the recommendation @adam-mateen made for option 1 fits the length (there's a longer prompt from func wantEC2TagDimensions) and is sufficient for the user to be aware of aware of the CollectD installation. Shall I proceed with the refactor for option 1?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the wording is sufficient. I'd say go for it

if yes {
collection := config.MetricsConf().Collection()
collection.CollectD = new(collectd.CollectD)