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

[dev mode] fallback to /tmp to dump pstats #2934

Merged
merged 1 commit into from
Oct 20, 2016
Merged

Conversation

hkaj
Copy link
Member

@hkaj hkaj commented Oct 19, 2016

What does this PR do?

If dumping pstats in the current dir fails, fall back to /tmp.

Motivation

User noticed that they didn't get the dumps because the agent was trying to write to / as the default path is relative and they were probably running it with supervisor.

Additional Notes

This was just a minor bug as the agent should not run in dev mode in production (it ruins its perf) and usually runs in the foreground from a directory where it has write access.

Copy link
Member

@degemer degemer left a comment

Choose a reason for hiding this comment

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

Why not always dump it in a tempfile.mkstemp ? (https://docs.python.org/2/library/tempfile.html#tempfile.mkstemp)
Something like

file_handle, file_path = tempfile.mkstemp(prefix='collector-statsd-', suffix='.dmp')
file_handle.close()
ps.dump_stats(file_path)

Since the path is in the debug log anyway.

But it'd be quite annoying when running in foreground. Try the current directory and then dump in a temp directory ?

At least don't hardcode /tmp, tempfile.gettempdir() is there for this. 📁

log.debug("Pstats dumps are enabled. Dumping pstats output to {0}".format(self.STATS_DUMP_FILE))
except IOError:
log.debug('Dumping pstats output to {} failed, using /tmp instead.'.format(self.STATS_DUMP_FILE))
ps.dump_stats(urljoin('/tmp/', self.STATS_DUMP_FILE))
Copy link
Member

Choose a reason for hiding this comment

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

os.path.join !

@hkaj hkaj force-pushed the haissam/dev-mode-fix-dump branch from 4b1e6c6 to 60b0b64 Compare October 20, 2016 13:40
@hkaj
Copy link
Member Author

hkaj commented Oct 20, 2016

thanks for the review @degemer
I updated according to your comments and found&fixed another related bug while testing.

Copy link
Member

@degemer degemer left a comment

Choose a reason for hiding this comment

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

Nearly there, I think you actually added another bug while fixing the old one. 😛

The backup dump part looks good. Tested it, worked fine.

self.collector_profile_interval = self._agentConfig.get('collector_profile_interval',
DEFAULT_COLLECTOR_PROFILE_INTERVAL)
try:
self.collector_profile_interval = int(self._agentConfig.get('collector_profile_interval'))
Copy link
Member

Choose a reason for hiding this comment

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

The default is still needed in the get, otherwise it will always fail when not configured.

DEFAULT_COLLECTOR_PROFILE_INTERVAL)
try:
self.collector_profile_interval = int(self._agentConfig.get('collector_profile_interval'))
except ValueError:
Copy link
Member

Choose a reason for hiding this comment

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

I think it's a TypeError: TypeError: int() argument must be a string or a number, not 'NoneType'.

@hkaj hkaj force-pushed the haissam/dev-mode-fix-dump branch from 60b0b64 to 02f0220 Compare October 20, 2016 14:54
@hkaj hkaj force-pushed the haissam/dev-mode-fix-dump branch from 02f0220 to a32e81b Compare October 20, 2016 14:56
@hkaj
Copy link
Member Author

hkaj commented Oct 20, 2016

doh! I added the default back. The exception is a ValueError if collector_profile_interval is provided but not a string we can turn into an int. (try with int('foo'))

Copy link
Member

@degemer degemer left a comment

Choose a reason for hiding this comment

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

🎉

@hkaj hkaj merged commit ea14e8b into master Oct 20, 2016
@hkaj hkaj deleted the haissam/dev-mode-fix-dump branch October 20, 2016 15:42
@olivielpeau olivielpeau added this to the 5.10.0 milestone Oct 24, 2016
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.

3 participants