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

Log messages are rendered in the v2 UI #8049

Merged
merged 4 commits into from Jul 17, 2019

Conversation

@gshuflin
Copy link
Contributor

commented Jul 13, 2019

Problem

Addresses [V2 UI] Pipe Rust Logger to the V2 UI #7906

Solution

As per the discussion on that issue, adds to the LOGGER singleton handles to EngineDisplay instances so that when a global log happens the log methods on EngineDisplay can also be called

@stuhood
Copy link
Member

left a comment

Thanks!

pub struct Logger {
pantsd_log: Mutex<MaybeWriteLogger<File>>,
stderr_log: Mutex<MaybeWriteLogger<Stderr>>,
show_rust_3rdparty_logs: AtomicBool,
engine_display_handles: Arc<Mutex<HashMap<Uuid, Arc<Mutex<EngineDisplay>>>>>,

This comment has been minimized.

Copy link
@stuhood

stuhood Jul 13, 2019

Member

I think that the outermost Arc here isn't necessary, because only this struct will have a reference to this Map.

Some(ref mut map) => {
for handle in map.values_mut() {
let log_string: String = format!("{}", record.args());
match handle.try_lock() {

This comment has been minimized.

Copy link
@stuhood

stuhood Jul 13, 2019

Member

The effect of using try_lock is I think that if anyone is currently holding the lock, a message will get dropped on the floor. We should try to avoid that if possible.

It looks like the Scheduler::execute method currently holds the lock on the EngineDisplay for the entire run, but that is precisely when we're hoping to render messages from logging. So that coarse locking will probably need to change into more fine-grained locking around individual method calls on the Display in execute.

@gshuflin gshuflin force-pushed the gshuflin:logger branch from 7dc0cfb to 1a2d8ba Jul 16, 2019

gshuflin added some commits Jul 12, 2019

Pipe rust logger to the v2 UI (#7906)
This commit makes the `LOGGER` singleton aware of handles to
`EngineDisplay` instances, so that when a log event happens `LOGGER` can
ensure that the v2 UI displays them.

@gshuflin gshuflin force-pushed the gshuflin:logger branch from 1a2d8ba to 07b0f63 Jul 16, 2019

@gshuflin gshuflin marked this pull request as ready for review Jul 16, 2019

@stuhood
Copy link
Member

left a comment

Thanks, looks great!

let mut display = display.lock();
display.finish();
}
LOGGER.deregister_engine_display(unique_handle);

This comment has been minimized.

Copy link
@stuhood

stuhood Jul 16, 2019

Member

Should probably swap deregister with display.finish(), so that we stop receiving log messages before we finish.

// If the number of ongoing tasks is less than the number of workers,
// fill the rest of the workers with empty string.
// TODO(yic): further improve the UI. https://github.com/pantsbuild/pants/issues/6666
for i in ongoing_tasks.len()..display_worker_count {
display.update(i.to_string(), "".to_string());
let worker_count = {

This comment has been minimized.

Copy link
@stuhood

stuhood Jul 16, 2019

Member

This is computed above as a well, but will not have changed.

This comment has been minimized.

Copy link
@gshuflin

gshuflin Jul 16, 2019

Author Contributor

Doesn't the call to update in the loop between these two uses of woker_count potentially change that count?

This comment has been minimized.

Copy link
@stuhood

stuhood Jul 16, 2019

Member

IIRC, the worker count is a construction-time parameter that isn't changed after construction.

Show resolved Hide resolved src/rust/engine/src/scheduler.rs

gshuflin added some commits Jul 16, 2019

Swap calls to `deregister_engine_display` and `finish`
Swap the order of these calls so that the `EngineDisplay` stops
receiving log messages before rather than after it finishes

@stuhood stuhood changed the title Logger Log messages are rendered in the v2 UI Jul 16, 2019

@stuhood stuhood merged commit 121ec6f into pantsbuild:master Jul 17, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@gshuflin gshuflin deleted the gshuflin:logger branch Jul 17, 2019

sammy-1234 pushed a commit to sammy-1234/pants that referenced this pull request Jul 18, 2019

Log messages are rendered in the v2 UI (pantsbuild#8049)
### Problem

Fixes pantsbuild#7906: "[V2 UI] Pipe Rust `Logger` to the V2 UI".

### Solution

As per the discussion on that issue, adds to the `LOGGER` singleton handles to `EngineDisplay` instances so that when a global log happens the log methods on `EngineDisplay` can also be called.

@gshuflin gshuflin restored the gshuflin:logger branch Jul 19, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.