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

Metricbeat mysql: Add config.epr.yml #17323

Merged
merged 2 commits into from
Mar 30, 2020

Conversation

mtojek
Copy link
Contributor

@mtojek mtojek commented Mar 30, 2020

This PR adds the config.epr.yml file to the Metricbeat mysql module.

I plan to use this file as source of configuration option for the import-beats script in https://github.com/elastic/package-registry project.

@mtojek mtojek requested review from ruflin and a team March 30, 2020 13:00
@mtojek mtojek self-assigned this Mar 30, 2020
@mtojek mtojek requested review from adoerrES and exekias and removed request for adoerrES March 30, 2020 13:00
@mtojek
Copy link
Contributor Author

mtojek commented Mar 30, 2020

If this file is required and cannot be change, WDYT about an additional one config.epr.yml that will be used only for importing purposes?

# - "galera_status"
- "galera_status"
Copy link
Contributor

Choose a reason for hiding this comment

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

this normally means that the metricset is disabled by default, do we really need to change 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.

I used this way (enabling metricset) to ensure that the configuration option is used with the metricset.
In this case, if I enable "galera_status", the script will include all configuration options related to it.

@exekias
Copy link
Contributor

exekias commented Mar 30, 2020

About reference config: it's only use is compiling the metricbeat.reference.yml, which holds a reference file of all default config values for the user. There is nothing really interacting with it.

I'm +1 on a config.epr.yml file if needed, perhaps it can have a more focused purpose and describe only what you need. I understand this is metricsets and config keys, similar to filebeat's manifest.yml in filesets

@mtojek
Copy link
Contributor Author

mtojek commented Mar 30, 2020

About reference config: it's only use is compiling the metricbeat.reference.yml, which holds a reference file of all default config values for the user. There is nothing really interacting with it.

I'm +1 on a config.epr.yml file if needed, perhaps it can have a more focused purpose and describe only what you need. I understand this is metricsets and config keys, similar to filebeat's manifest.yml in filesets

This will work for me too. I can adjust the PR.

@mtojek mtojek requested a review from exekias March 30, 2020 13:13
@mtojek mtojek changed the title Metricbeat mysql: Uncomment all fields in the reference config Metricbeat mysql: Add config.epr.yml Mar 30, 2020
@ruflin
Copy link
Member

ruflin commented Mar 30, 2020

I like the idea of having some assets in Beats that will simplify the migration. But the part I wonder is if this case we should perhaps directly jump to the end format, meaning having the manifest file instead with parts of the info?

@mtojek
Copy link
Contributor Author

mtojek commented Mar 30, 2020

I like the idea of having some assets in Beats that will simplify the migration. But the part I wonder is if this case we should perhaps directly jump to the end format, meaning having the manifest file instead with parts of the info?

@ruflin Having an entire manifest.yml in every dataset in beats might require lot of manual work. I'm not sure if this is a way we'd like to follow (I'm not declining this approach).

@ruflin
Copy link
Member

ruflin commented Mar 30, 2020

@mtojek Agree, and that manual work should probably directly happening when we review the packages. Just having a specific epr config file sounds also like a lot of additional work.

Copy link
Contributor

@exekias exekias left a comment

Choose a reason for hiding this comment

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

Let's get this in for the shake of moving MySQL integration forward. If we finally go for this approach we will need to document it after the first batch of integrations are in

@mtojek
Copy link
Contributor Author

mtojek commented Mar 30, 2020

@mtojek Agree, and that manual work should probably directly happening when we review the packages. Just having a specific epr config file sounds also like a lot of additional work.

Hmm, content of the config.epr.yml can be copied from a config.reference.yml, but the manifest needs to be rewritten.
Let's try to go with config.epr.yml. In the worst case, we'll rollback these files and apply a different option.

@mtojek mtojek merged commit 7fa15ff into elastic:master Mar 30, 2020
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.

None yet

3 participants