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

Fixes logging suppression caused by early logging. #56

Closed
wants to merge 1 commit into from

Conversation

rountree
Copy link

@rountree rountree commented Nov 4, 2024

Logging initialization assumes a missing SPINDLE_DEBUG environment variable means SPINDLE_DEBUG=0. Messages that are logged prior to that environment variable being set are lost and inadvertently suppress any further logging messages from that subsystem.

This patch sets SPINDLE_DEBUG=0 in the shell environment if that variable is missing from the flux environment. It also modifies the logging initialization process so that any messages logged before SPINDLE_DEBUG is set are sent through, but any subsequent message will trigger re-initialization until SPINDLE_DEBUG exists.

Fixes #55

Logging initialization assumes a missing SPINDLE_DEBUG environment
variable means SPINDLE_DEBUG=0.  Messages that are logged prior to
that environment variable being set are lost and inadvertently
suppress any further logging messages from that subsystem.

This patch sets SPINDLE_DEBUG=0 in the shell environment if that
variable is missing from the flux environment.  It also modifies
the logging initialization process so that any messages logged
before SPINDLE_DEBUG is set are sent through, but any subsequent
message will trigger re-initialization until SPINDLE_DEBUG exists.
@mplegendre
Copy link
Member

We need to leave debugging off if SPINDLE_DEBUG is unset. There's significant overhead/poor-scaling if debugging is enable. And file pollution, as it drops files in your PWD.
The typical way users will run will be without SPINDLE_DEBUG set. There's also several run modes that don't go through the flux plugin, so we can't just rely on setting/unsetting it there.

If you're going to insert a compile-time instrumentation that requires debug tracing, then add a compile-time flag that turns debugging on by default. I can help add that flag, if that would be easier.

@rountree
Copy link
Author

rountree commented Nov 4, 2024

Point taken wrt scaling and file pollution. I can imagine a solution where messages (with their priority) are buffered until initialization occurs, then parsed to see if they're logged or discarded. It's not wholly unlike trying to make printf() work in _start() before your file descriptors are set up, and, perhaps like that, it's not a problem worth solving (or not worth solving this week).

@mplegendre
Copy link
Member

But then you've got to decide how long to buffer. And you're still paying the internal overhead of logging.

I've got a upcoming commit that changes Spindle's config management. It's already got some other changes to autoconf configure. I'll throw in a configure --with-default-debugging-level=X with that commit, and we can set that when adding the -finstrument.

I'm going to close this. We can re-open if the discussion circles back to this kind of approach.

@mplegendre mplegendre closed this Nov 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

All front-end debugging messages suppressed by early initial message
3 participants