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

[util] remove LaconicFilter #2605

Merged
merged 2 commits into from
Jun 17, 2016
Merged

[util] remove LaconicFilter #2605

merged 2 commits into from
Jun 17, 2016

Conversation

degemer
Copy link
Member

@degemer degemer commented Jun 15, 2016

This filter may be useful, but it:

  • complexifies debug when you don't know it exists ("Why don't I see
    this line multiple times in the logs"),
  • adds a lot of noise when trying to track memory utilization,
  • is completely unexpected.

@degemer degemer added the core label Jun 15, 2016
@degemer degemer added this to the 5.9.0 milestone Jun 15, 2016
@olivielpeau
Copy link
Member

Wholly agree with your reasons to remove the filter!

Went through the checks that are currently using the filter to check that they wouldn't start flooding the logs, and IMO there are 2 places, in checks/system/unix.py, where we should change the way we log errors (because we currently log an exception for every line of output that we can't parse):

Apart from this the PR looks good to me! 🚀

@remh
Copy link

remh commented Jun 16, 2016

While we are at it, it could be useful to increase the max logging size:
https://github.com/DataDog/dd-agent/blob/master/config.py#L43

As sometimes when investigating issues, logs already got rotated.

@degemer degemer force-pushed the quentin/delete-laconic branch from d9ef1ba to 77e3933 Compare June 16, 2016 18:19
@degemer
Copy link
Member Author

degemer commented Jun 16, 2016

Updated as per your comments. I guess dogstream is one other thing that could flood the logs, but...

@olivielpeau
Copy link
Member

Yeah... The custom dogstream parsers that log a lot could start flooding the collector logs, but I still think it's better to remove the filter.

Should be up to the custom parsers' implementations to log things responsibly IMO...

@degemer degemer force-pushed the quentin/delete-laconic branch from 77e3933 to 22b9caf Compare June 16, 2016 19:17
self.logger.exception("Cannot parse %s", proc_meminfo)
parse_error = True
if parse_error:
self.logger.error("Error parsing %s", proc_meminfo)
Copy link
Member

Choose a reason for hiding this comment

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

this line is over-indented 😛

@olivielpeau
Copy link
Member

Had one nitpick, feel free to merge once it's fixed! 🎆

degemer added 2 commits June 16, 2016 17:55
This filter might be sometimes useful, but it:
- complexifies debug when you don't know it exists ("What don't I see
  this line multiple times in the logs"),
- adds a lot of noise when trying to track memory utilization,
- is completely unexpected.

This also muzzle two noisy log lines.
@degemer degemer force-pushed the quentin/delete-laconic branch from 22b9caf to d8cb076 Compare June 16, 2016 21:56
@degemer
Copy link
Member Author

degemer commented Jun 16, 2016

Thanks for the review, waiting for the 💚 CI before merging.

@degemer degemer merged commit dfc2211 into master Jun 17, 2016
@degemer degemer deleted the quentin/delete-laconic branch June 17, 2016 14:33
olivielpeau added a commit that referenced this pull request Aug 19, 2016
Since we removed the LaconicFilter (see #2605) the collector is filled
with `Running check xxxx` messages.

Let's log it at the `info` level only on the first run (since it can
be useful to quickly detect if a check is consistently running too
slowly), but only log it at the `debug` level afterwards.
olivielpeau added a commit that referenced this pull request Aug 19, 2016
Since we removed the LaconicFilter (see #2605) the collector is filled
with `Running check xxxx` messages.

Let's log it at the `info` level only on the first run (since it can
be useful to quickly detect if a check is consistently running too
slowly), but only log it at the `debug` level afterwards.
olivielpeau added a commit that referenced this pull request Aug 19, 2016
…ly (#2772)

* [collector][logging] Log check starts with info level on first run only

Since we removed the LaconicFilter (see #2605) the collector is filled
with `Running check xxxx` messages.

Let's log it at the `info` level only on the first run (since it can
be useful to quickly detect if a check is consistently running too
slowly), but only log it at the `debug` level afterwards.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants