tokio-rs/console

assertion failed: meta.is_event()

dinhani opened this issue · 3 comments

What crate(s) in this repo are involved in the problem?

console-api

What is the issue?

console-api is panicking when running in debug mode in this debug_assert!

No idea if this is expected or not, but I identified that an event of kind HINT is reaching that point and triggering the assertion to fail.

It comes from the external crate jsonrpsee-core, so I cannot easily change it.

Metadata { 
  name: "enabled /home/renato/.cargo/registry/src/index.crates.io-6f17d22bba15001f/jsonrpsee-core-0.22.5/src/tracing.rs:13", 
  target: "jsonrpsee_core::tracing::client", 
  level: Level(Trace), 
  module_path: "jsonrpsee_core::tracing::client", 
  location: /home/renato/.cargo/registry/src/index.crates.io-6f17d22bba15001f/jsonrpsee-core-0.22.5/src/tracing.rs:13, 
  fields: {}, 
  callsite: Identifier(0x559831eadc48), 
  kind: Kind(HINT) 
}

How can the bug be reproduced?

let client = HttpClientBuilder::default().build(jsonrpc_url).unwrap();
let _ = client.request::<bool, Vec<()>>("net_listening", vec![]).await

Logs, error output, etc

thread 'tokio' panicked at /home/renato/projects/console/console-api/src/common.rs:50:13:
assertion failed: meta.is_event()

Versions

├── console-subscriber v0.2.0
│   ├── console-api v0.6.0

Possible solution

Maybe filtering out invalid metadata kinds before calling the function or using TryFrom instead.
The function is being called from aggregator.rs in console-subscriber.

Additional context

No response

Would you like to work on fixing this bug?

yes

hds commented

@dinhani Thanks for your issue report!

I would say that this is definitely incorrect on console-api side, even in debug mode we should be more resilient.

This will only happen if the console-subscriber is receiving traces that aren't from Tokio, since in the Tokio instrumentation we only use events and spans, but if we're going to have this assertion in place, we should be filtering out non-event/non-span callsites and not sending them on to the aggregator (we won't do anything with them anyway).

If you're interested in working on this, I propose that we check that the metadata received by register_callsite is either an event or a span, and otherwise we return Interest::never() and don't do any more:

fn register_callsite(&self, meta: &'static Metadata<'static>) -> subscriber::Interest {

We could add a test for this behaviour, although it looks like our test set-up doesn't fail the test when the subscriber thread panics, which is weird. I'll look at that separately.

I tested my project with the suggested change at register_callsite and it worked. There are no more panics.

I sent the suggested PR, but I still need to check how to test it properly in this project.

hds commented

This has been fixed in #554. Thanks for fixing your own report @dinhani!