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

Set default metricsets for system module #6689

Merged
merged 1 commit into from
Apr 4, 2018

Conversation

jsoriano
Copy link
Member

See #6668

Set as default metricset the ones that appear in example configuration enabled and don't require further configuration.

@@ -55,6 +47,16 @@ metricbeat.modules:
period: 15m
metricsets:
- uptime
#- core
Copy link
Contributor

Choose a reason for hiding this comment

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

We probably can remove this and the following lines from the config.yml. Or would you keep them in the advertise the others?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I though it could be fine to leave them for visibility. I could also leave them in another commented-out module block.

Copy link
Member Author

Choose a reason for hiding this comment

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

I have finally added another commented-out module block for these non-default metricsets.

@@ -106,6 +107,9 @@ metricbeat.modules:
# to false.
#process.include_cpu_ticks: false

# Raid mount point to monitor
#raid.mount_point: '/'
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for completing this.

@jsoriano jsoriano force-pushed the system-default-metricset branch 2 times, most recently from e9f1851 to 98c6cd3 Compare March 29, 2018 10:39
@jsoriano
Copy link
Member Author

Fixed a typo that broke compilation, and another one in the documentation.

@@ -28,3 +23,9 @@
period: 15m
metricsets:
- uptime

# - module: system
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: There is a space after # which we normally don't have except for comments.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, fixed.

process.include_top_n:
by_cpu: 5 # include top 5 processes by CPU
by_memory: 5 # include top 5 processes by memory

#- module: system
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if the indentation is correct if the # is removed here. It should be 2 spaces before the #.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think identation is correct, if the # is removed it stays at the same level as other blocks.

Copy link
Contributor

Choose a reason for hiding this comment

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

ah, you are right.

@jsoriano jsoriano force-pushed the system-default-metricset branch from 98c6cd3 to f836de7 Compare April 3, 2018 10:07
process.include_top_n:
by_cpu: 5 # include top 5 processes by CPU
by_memory: 5 # include top 5 processes by memory

#- module: system
Copy link
Contributor

Choose a reason for hiding this comment

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

ah, you are right.

#- diskio
#- socket
processes: ['.*']
# processes: ['.*']
Copy link
Contributor

Choose a reason for hiding this comment

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

Two thins here:

  • It seemed we changed the default here from having processes filter enabled to disabled
  • If it is commented out, the space after the # should be removed.

I would keep the previous config if possible.

Copy link
Member Author

Choose a reason for hiding this comment

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

I commented it out because this value is already the default one, I could remove it as it is already in the reference config.

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 remove it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, done.

@jsoriano jsoriano force-pushed the system-default-metricset branch from f836de7 to dc5bfb9 Compare April 4, 2018 08:50
Copy link
Contributor

@ruflin ruflin left a comment

Choose a reason for hiding this comment

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

WFG

@ruflin ruflin merged commit d2ff4bf into elastic:master Apr 4, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants