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

Fix and extend logging of light modules load errors #14706

Merged
merged 11 commits into from
Mar 3, 2020

Conversation

jsoriano
Copy link
Member

@jsoriano jsoriano commented Nov 22, 2019

Two things are solved here:

  • Errors were being logged with logp.Error that doesn't actually log anything, use a local logger and its Errorf method instead.
  • Log errors also when listing modules for string representation.

This was hiding errors, what was specially confusing when permissions are not correct during development and testing, now errors like this one are logged if permissions are not correct:

2019-11-22T12:03:51.398+0100	ERROR	mb/lightmodules.go:107	failed to load module config for module 'cockroachdb': failed to load module configuration from '/home/jaime/gocode/src/github.com/elastic/beats/x-pack/metricbeat/module/cockroachdb/module.yml': config file ("/home/jaime/gocode/src/github.com/elastic/beats/x-pack/metricbeat/module/cockroachdb/module.yml") can only be writable by the owner but the permissions are "-rw-rw-r--" (to fix the permissions use: 'chmod go-w /home/jaime/gocode/src/github.com/elastic/beats/x-pack/metricbeat/module/cockroachdb/module.yml')

Some errors have been rephrased so logged errors start with uppercase, and "failed to" is not logged in all nested errors.

@jsoriano jsoriano added bug review Metricbeat Metricbeat needs_backport PR is waiting to be backported to other branches. [zube]: In Review Team:Integrations Label for the Integrations team v7.6.0 v7.5.1 labels Nov 22, 2019
@jsoriano jsoriano self-assigned this Nov 22, 2019
@jsoriano
Copy link
Member Author

jsoriano commented Nov 22, 2019

This is making many test to fail because they find errors in logs, I am adding BEAT_STRICT_PERMS=false to the beat service in docker-compose as is done in filebeat.

@exekias
Copy link
Contributor

exekias commented Nov 26, 2019

what will happen to folks running tests outside docker? we need to think about that case too

@jsoriano
Copy link
Member Author

what will happen to folks running tests outside docker? we need to think about that case too

Logging-wise it would be the same.

Regarding permissions, when running tests outside docker (and in systems with default umask != 0022 as most ubuntu distros) it is usual to require BEAT_STRICT_PERMS=false in may tests, specially for filebeat, and now also for metricbeat with light modules. We are setting the environment variable in most tests related to light modules for convenience.

@kaiyan-sheng
Copy link
Contributor

@jsoriano Seems like this PR is ready to go besides the conflict 😄 I saw this during triage party time. Could you take a look at this one again when you get a chance please? Thanks!

@jsoriano
Copy link
Member Author

jsoriano commented Mar 2, 2020

@kaiyan-sheng good catch 😄 The original issue was solved in other way, but the fixes in logging may still be needed, I will take a look.

@jsoriano
Copy link
Member Author

jsoriano commented Mar 2, 2020

Revisited, this would be ready for another review.

Copy link
Contributor

@kaiyan-sheng kaiyan-sheng left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@jsoriano jsoriano added the v7.7.0 label Mar 2, 2020
@jsoriano jsoriano requested a review from a team as a code owner March 2, 2020 20:04
@jsoriano
Copy link
Member Author

jsoriano commented Mar 2, 2020

There was a bug causing regular files in the module directory to be listed as modules with no metricsets. I have fixed this and added a test to cover this case. This would need another review.

Copy link
Contributor

@kaiyan-sheng kaiyan-sheng left a comment

Choose a reason for hiding this comment

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

lgtm!

@jsoriano jsoriano merged commit 10da237 into elastic:master Mar 3, 2020
@jsoriano jsoriano deleted the fix-lightmodules-permissions branch March 3, 2020 20:35
@jsoriano jsoriano removed the needs_backport PR is waiting to be backported to other branches. label Mar 3, 2020
jsoriano added a commit to jsoriano/beats that referenced this pull request Mar 3, 2020
Two things are solved here:

* Errors were being logged with logp.Error that doesn't actually log
  anything, use a local logger and its Errorf method instead.
* Log errors also when listing modules for string representation.

This was hiding errors, what was specially confusing when permissions
are not correct during development and testing, now errors are logged
if permissions are not correct.

Some errors have been rephrased so logged errors start with uppercase,
and "failed to" is not logged in all nested errors.

(cherry picked from commit 10da237)
jsoriano added a commit that referenced this pull request Mar 4, 2020
Two things are solved here:

* Errors were being logged with logp.Error that doesn't actually log
  anything, use a local logger and its Errorf method instead.
* Log errors also when listing modules for string representation.

This was hiding errors, what was specially confusing when permissions
are not correct during development and testing, now errors are logged
if permissions are not correct.

Some errors have been rephrased so logged errors start with uppercase,
and "failed to" is not logged in all nested errors.

(cherry picked from commit 10da237)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Metricbeat Metricbeat review Team:Integrations Label for the Integrations team v7.7.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants