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

Add a custom "TRACE" log level for even more verbosity. #4672

Merged
merged 10 commits into from Oct 4, 2017

Conversation

Projects
None yet
6 participants
@dannon
Member

dannon commented Sep 21, 2017

Import logging as usual and within the galaxy namespace you should be able to use it with log.trace().

Includes one example swapping a statement over.

I found the existing NullHandler logging bits in galaxy and added this there, though we could maybe move both to galaxy.logging if we think our logging config might get much more sophisticated.

@jmchilton

This comment has been minimized.

Member

jmchilton commented Sep 21, 2017

Can we do this differently - this is some nice magic and it works well for "Galaxy the application" - it is very elegant. The downside is I don't think monkey patching a standard library module is a good behavior for a library - so I'd like not to do this in galaxy-lib for instance. I'd prefer not not monkey the logger and just import a "trace" function from the galaxy.util module that takes the arguments and the logger and does this.

If I'm voted down on this can we at least allow both methods of tracing and then use the alternative, less elegant galaxy.util.trace for methods in galaxy-lib?

@dannon

This comment has been minimized.

Member

dannon commented Sep 21, 2017

@jmchilton Yeah, I don't mind. I actually started out with something similar in galaxy.logging, and then realized how this might work so much more cleanly. It's a good point about galaxy-lib, though; I should be able to tweak this pretty easily.

@dannon

This comment has been minimized.

Member

dannon commented Sep 21, 2017

(and, I think we can actually drop the other pre-existing logging patch here -- that's only for python2.6 which we don't support anymore)

@dannon

This comment has been minimized.

Member

dannon commented Sep 22, 2017

Refactored, removed some python 2.6 cruft as well. Looks like I may need to touch up a few things though, so, WIPing it for a bit.

(renaming log to logging backfired in a big way w/ relative imports in py27)

@dannon dannon added status/WIP and removed status/review labels Sep 22, 2017

@nsoranzo

This comment has been minimized.

Member

nsoranzo commented Sep 25, 2017

@dannon Now that #4671 has been merged forward, this PR is conflicting.

@dannon

This comment has been minimized.

Member

dannon commented Sep 25, 2017

@nsoranzo Yeah, I'll fix it up. I'm mostly out today due to jury duty.

@dannon dannon added status/review and removed status/WIP labels Oct 3, 2017

@jmchilton jmchilton merged commit c49adb1 into galaxyproject:dev Oct 4, 2017

6 checks passed

api test Build finished. 292 tests run, 4 skipped, 0 failed.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
framework test Build finished. 161 tests run, 0 skipped, 0 failed.
Details
integration test Build finished. 46 tests run, 0 skipped, 0 failed.
Details
lgtm analysis: JavaScript No alert changes
Details
toolshed test Build finished. 579 tests run, 0 skipped, 0 failed.
Details
@jmchilton

This comment has been minimized.

Member

jmchilton commented Oct 4, 2017

Thanks for working on this @dannon, looks very solid!

@dannon

This comment has been minimized.

Member

dannon commented Oct 4, 2017

Sure, thanks for the nudge towards doing it this way; I'm happier with how it turned out.

@natefoo

This comment has been minimized.

Member

natefoo commented Oct 5, 2017

So, one side effect of this change is that when switching from dev to any older release branch, lib/galaxy/util/logging/__init__.pyc is left behind, resulting in AttributeError: 'module' object has no attribute 'getLogger' in lots of galaxy.util modules (the ones that have been changed to use absolute imports in this PR) on startup. This can be worked around by doing a find lib -name \*.pyc -print0 | xargs -0 rm, but that's a pain to have to do every time you switch branches (although it's occasionally been necessary at other times, but it's never been required that I can recall).

It can also be worked around by backporting absolute_import to old releases, but I am not sure that recovering from an annoyance only encountered when switching branches in development is in the spirit of stable bugfixes. Although it's worth noting that it would break release downgrades from 18.01 to 17.09 or older, if someone wanted to do that.

We could also add the find (and probably locate empty dirs and rmdir them) to run.sh and backport that, but it'd further increase startup times for something that is rarely necessary.

Other ideas?

@dannon

This comment has been minimized.

Member

dannon commented Oct 5, 2017

I dunno about backporting the fix or renaming it, since this is just a temporary 'problem'. The naming seems right as-is, and you're right, the backport doesn't really feel like a stable bugfix.

One option is a commit hook to act on changing branches, but those aren't really easily shared :/

edit: I definitely do feel like we had this happen in the past and we just used it as a moment to educate folks (developers encountering it) about how to clean up old .pyc.

@martenson

This comment has been minimized.

Member

martenson commented Oct 5, 2017

I disagree with temporary here. Eevery committer will experience this daily for next 1 month, and then weekly for next 6 months and then it will make security patches even more annoying.

@jmchilton

This comment has been minimized.

Member

jmchilton commented Oct 5, 2017

Seems like a safe thing to back port and we have backported for similar problems before. #4565 was backported really far and I think it was a solid choice. There will be admins that hit this problem IMO - it won't just be developers.

@dannon

This comment has been minimized.

Member

dannon commented Oct 5, 2017

I'm fine with the backport, if folks think something must be done. We have definitely had other (temporary) similar situations (in the past and we did not go to these lengths, though. (find . -name "*.pyc" -delete solved problems)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment