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

Thread-local logging #7690

Merged

Conversation

Projects
None yet
3 participants
@illicitonion
Copy link
Contributor

commented May 9, 2019

Rust Logger writes either to stderr or the pantsd log file depending on which the calling thread has registered itself as.

This avoids pantsd logging statements from ending up in stderr, particularly when a repl is active.

@illicitonion illicitonion requested review from stuhood and blorente May 9, 2019

@illicitonion illicitonion force-pushed the twitter:dwagnerhall/thread-local-logging branch from 0d7ca2e to b703e8e May 9, 2019

@stuhood

stuhood approved these changes May 9, 2019

Copy link
Member

left a comment

Thanks!

I'm fine with the future/task patch, although it might be good to ping Carl or Eliza about it to confirm?

static TASK_DESTINATION: Mutex<Option<BitFlags<Destination>>> = Mutex::new(None)
}

pub fn set_task_logging_destination(destination: BitFlags<Destination>) {

This comment has been minimized.

Copy link
@stuhood

stuhood May 9, 2019

Member

A doc blob somewhere that explains the overall approach (starting by inlining the PR description) would be handy.

This comment has been minimized.

Copy link
@illicitonion

illicitonion May 13, 2019

Author Contributor

Done (on the Destination enum)

@@ -222,6 +222,47 @@ impl Core {
pub fn http_client(&self) -> reqwest::r#async::Client {
self.store_and_command_runner_and_http_client.get().2
}

///
/// Start running a Future on a tokio Runtime.

This comment has been minimized.

Copy link
@stuhood

stuhood May 9, 2019

Member

Somewhere in here we should mention explicitly the reason for the overrides (setting logging state).

This comment has been minimized.

Copy link
@illicitonion

illicitonion May 13, 2019

Author Contributor

Done

@@ -871,6 +875,14 @@ pub extern "C" fn flush_log() {
LOGGER.flush();
}

#[no_mangle]
pub extern "C" fn override_thread_logging_destination(new_destination: u8) {

This comment has been minimized.

Copy link
@stuhood

stuhood May 9, 2019

Member

Do you anticipate this being used to actually set both bit flags... ie: "write to both"? If not, you can use the enum directly in the API like so:

# Get.
response.tag = self._lib.Get
response.get = (
TypeId(c.to_id(res.product)),
c.to_value(res.subject),
c.identify(res.subject),
)

This comment has been minimized.

Copy link
@blorente

blorente May 9, 2019

Contributor

+1 to the curiosity about this.

This comment has been minimized.

Copy link
@illicitonion

illicitonion May 13, 2019

Author Contributor

Switched to exclusive. Went down a bit of a cbindgen rabbit-hole to get there.

@@ -44,7 +44,7 @@ pub struct Core {
pub tasks: Tasks,
pub rule_graph: RuleGraph,
pub types: Types,
pub runtime: Resettable<Arc<RwLock<Runtime>>>,
runtime: Resettable<Arc<RwLock<Runtime>>>,

This comment has been minimized.

Copy link
@stuhood
Show resolved Hide resolved src/rust/engine/src/lib.rs Outdated
@@ -871,6 +875,14 @@ pub extern "C" fn flush_log() {
LOGGER.flush();
}

#[no_mangle]
pub extern "C" fn override_thread_logging_destination(new_destination: u8) {

This comment has been minimized.

Copy link
@blorente

blorente May 9, 2019

Contributor

+1 to the curiosity about this.

@illicitonion illicitonion force-pushed the twitter:dwagnerhall/thread-local-logging branch 2 times, most recently from 8fe1ed6 to 79ec160 May 13, 2019

@illicitonion illicitonion changed the title WIP: Thread-local logging Thread-local logging May 13, 2019

@illicitonion illicitonion force-pushed the twitter:dwagnerhall/thread-local-logging branch 2 times, most recently from acacbdf to 3d26e96 May 13, 2019

illicitonion added some commits May 3, 2019

Review comments
* Encapsulate a bit better
* Add docs (both overview and per-method)
* Use an exclusive enum instead of bitflags. Also, update cbindgen so
  this actually works.

@illicitonion illicitonion force-pushed the twitter:dwagnerhall/thread-local-logging branch from 3d26e96 to 15c2fd8 May 13, 2019

@stuhood
Copy link
Member

left a comment

Thanks again!

///
/// Run a Future and return its resolved Result.
///
/// This should never be called from in a Future context, or any context where anyone may want to

This comment has been minimized.

Copy link
@stuhood

stuhood May 13, 2019

Member

Not worth blocking merge on probably, but if there are upstream docs to link on this topic, that would be handy.

@illicitonion illicitonion merged commit a731138 into pantsbuild:master May 16, 2019

1 check failed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details

@illicitonion illicitonion deleted the twitter:dwagnerhall/thread-local-logging branch May 16, 2019

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