-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
[docker_daemon] detect bogus pids and dont traverse /proc for them. #237
Conversation
SDK sibling for: DataDog/dd-agent#3205 + DataDog/dd-agent#3218 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, we just need to sync the merge with the dd-agent PR for dockerutil.
docker_daemon/check.py
Outdated
continue | ||
self._report_net_metrics(container, tags) | ||
except BogusPIDException: | ||
self.log.exception('Unable to report cgroup metrics.') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need log.exception
? We know the stacktrace, we raise this error in only one place, unless I'm missing something?
Could we specify a bit the error message? Since we catch a BogusPIDException, couldn't we share more info than just a generic message with the user?
^ Feel free to disregard those messages, I obviously don't know the context. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nah that's a good point: https://github.com/DataDog/dd-agent/pull/3205/files#r101987067
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do agree there's no need to log the stack trace at all as this only happens here.
817a05c
to
ae90a80
Compare
No description provided.