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

assertion failed: meta.is_event() #553

Closed
dinhani opened this issue May 23, 2024 · 3 comments
Closed

assertion failed: meta.is_event() #553

dinhani opened this issue May 23, 2024 · 3 comments
Labels
S-bug Severity: bug

Comments

@dinhani
Copy link
Contributor

dinhani commented May 23, 2024

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

@dinhani dinhani added the S-bug Severity: bug label May 23, 2024
@hds
Copy link
Collaborator

hds commented May 24, 2024

@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.

@dinhani
Copy link
Contributor Author

dinhani commented May 24, 2024

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 pushed a commit that referenced this issue Jun 6, 2024
As described in #553, there are parts of code that expects tracing
`Metadata` to be of type `EVENT` or `SPAN`.

This PR ensure that any tracing metadata with a different type will be
ignored instead of panicking when running in debug mode when converting
from `tracing_core::Metadata`:
https://github.com/tokio-rs/console/blob/60bcf87537d414b21efbd84159207f9ecda5e914/console-api/src/common.rs#L48
@hds
Copy link
Collaborator

hds commented Jun 7, 2024

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

@hds hds closed this as completed Jun 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-bug Severity: bug
Projects
None yet
Development

No branches or pull requests

2 participants