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

Client doesn't log errors #239

Closed
WhyNotHugo opened this issue Aug 7, 2020 · 23 comments
Closed

Client doesn't log errors #239

WhyNotHugo opened this issue Aug 7, 2020 · 23 comments

Comments

@WhyNotHugo
Copy link

My code is fairly simply, and copying the panic example from the docs doesn't log anything (nothing shows up on Sentry, and it doesn't seem to pause to send the events).

//...

#[tokio::main]
async fn main() -> Result<()> {
    let version = env!("VERSION").to_string();
    info!("Version {}", version);

    let sentry = sentry::init((
        "https://XXXXXX@o58074.ingest.sentry.io/YYYYY",
        sentry::ClientOptions {
            release: Some(format!("sync@{}", version).into()),
            ..Default::default()
        },
    ));

    configure_logger();

    if sentry.is_enabled() {
        debug!("Sentry is enabled");
    }

    //...

    panic!("Everything is on fire!");
//...
[2020-08-07 22:42:57] [DEBUG][sync_tool] Sentry is enabled
thread 'main' panicked at 'Everything is on fire!', src/main.rs:165:9
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

No event on sentry.

Any hints? This is pretty close to a copy-paste from the docs on all sides.

@GeorgeHahn
Copy link

I observe the same issue on versions 19.0 and 0d9d1d0. I've traced through the panic handler and can see the sentry handler run, but the event doesn't show up in the web interface.

@Swatinem
Copy link
Member

I’m able to reproduce this, but it seems quite random, will investigate.

@ErichDonGubler
Copy link
Contributor

This hasn't been resolved with 0.20.0, has it?

@Swatinem
Copy link
Member

Swatinem commented Sep 9, 2020

I had hoped that it has. Can you provide any more Info on this?

@ErichDonGubler
Copy link
Contributor

ErichDonGubler commented Sep 9, 2020

I unfortunately am not familiar with usage of sentry as a crate, though I'm now responsible for a codebase that uses it with @NZXTCorp. I would defer information gathering to @GeorgeHahn and @WhyNotHugo, since I haven't reproduced the issue myself yet -- I'm just assessing risk with issues for my team right now. :) I'll try to reproduce this next week if I can, though!

@Mastermindaxe
Copy link

Mastermindaxe commented Sep 28, 2020

Can confirm that this is still happening even with the minimal example that is stated in the docs. Using version 0.20.1. I'm trying to see if I can fix it by going back a few versions

EDIT: I tried versions 0.19, 0.18, 0.17, 0.16 and 0.15. All versions fail to submit the panic to Sentry.
Using this code:

fn main() {
    let _guard = sentry::init("https://key@sentry.io/42"); // Using my sentry intake adress ofc
    panic!("Everything is on fire!");
}

@jan-auer
Copy link
Member

@Mastermindaxe Up to SDK version 0.18, you have to register the panic handler using sentry::integrations::panic::register_panic_handler.

Beginning with SDK version 0.19, the panic integration will take care of that. However, the above bug apparently still applies.

@Mastermindaxe
Copy link

Ah okay! Disregard my testing up to version 0.18 then ^^

@aramperes
Copy link
Contributor

aramperes commented Oct 2, 2020

I was encountering this in both 0.19 and 0.20. It was working on my local system, but failing on my deployment (in a debian Docker container). In my case, the solution was to install the ca-certificates package.

To clarify: by default, HTTPS calls will fail with Hyper in a Debian Docker container because no certificates are trusted out-of-the-box. So make sure you apt-get update && apt-get install -y "libssl1.1" ca-certificates in your Dockerfile.

This was pretty hard to debug since sentry/hyper isn't logging the SSL error by default. I turned on RUST_LOG=debug and this was the output

Before the fix

 2020-10-02T22:27:35.393Z ERROR my_service > <Error message>
 2020-10-02T22:27:35.394Z DEBUG hyper::client::connect::dns > resolving host="<sentry URL>" 
 2020-10-02T22:27:35.404Z DEBUG hyper::client::connect::http > connecting to IP:443 
 2020-10-02T22:27:35.408Z DEBUG hyper::client::connect::http > connected to IP:443 
 2020-10-02T22:27:35.414Z DEBUG hyper::proto::h1::io         > flushed 246 bytes 
 2020-10-02T22:27:35.415Z DEBUG hyper::proto::h1::io         > flushed 7934 bytes 

After the fix

 2020-10-02T22:27:35.393Z ERROR my_service > <Error message>
 2020-10-02T22:27:35.394Z DEBUG hyper::client::connect::dns > resolving host="<sentry URL>" 
 2020-10-02T22:27:35.404Z DEBUG hyper::client::connect::http > connecting to IP:443 
 2020-10-02T22:27:35.408Z DEBUG hyper::client::connect::http > connected to IP:443 
 2020-10-02T22:27:35.414Z DEBUG hyper::proto::h1::io         > flushed 246 bytes 
 2020-10-02T22:27:35.415Z DEBUG hyper::proto::h1::io         > flushed 7934 bytes 
 2020-10-02T22:27:35.456Z DEBUG hyper::proto::h1::io         > read 1014 bytes 
 2020-10-02T22:27:35.456Z DEBUG hyper::proto::h1::io         > parsed 17 headers 
 2020-10-02T22:27:35.456Z DEBUG hyper::proto::h1::conn       > incoming body is content-length (41 bytes) 
 2020-10-02T22:27:35.456Z DEBUG hyper::proto::h1::conn       > incoming body completed 
 2020-10-02T22:27:35.456Z DEBUG hyper::client::pool          > pooling idle connection for ("https", <sentry url>) 
 2020-10-02T22:27:35.456Z DEBUG reqwest::async_impl::client  > response '200 OK' for https://<sentry url>

It would be nice if you didn't need the debug-logs feature (or debug option) to find fatal Sentry errors. Perhaps an alternative macro, e.g. sentry_critical!, could be used to log straight to stderr?

@mattbeedle
Copy link

I'm also having issues with this. If I add:

sentry::capture_message("Hello World!", sentry::Level::Info);

before the panic! then both the captured message and the panic are logged in sentry. Without the capture_message nothing is logged.

@dconnolly
Copy link

I'm reproducing this on macos, same code succeeds on debian with the ca-certificates package installed.

@dconnolly
Copy link

This example does not send events for the capture_message nor the panic:

fn main() {
    let _guard = sentry::init(
        sentry::ClientOptions {
            debug: true,
            ..Default::default()
        }, 
    );

    let next = std::panic::take_hook();
    std::panic::set_hook(Box::new(move |info| {
        println!("Caught panic");
        sentry::integrations::panic::panic_handler(info);
        next(info);
    }));

    sentry::capture_message("Hello World!", sentry::Level::Info);

    panic!("Everything is on fire!");
}

Cargo.toml:

[dependencies]
sentry = { version = "0.21.0", default-features = false, features = ["backtrace", "contexts", "panic", "reqwest", "rustls"] }

@yaahc
Copy link

yaahc commented Dec 8, 2020

following up on @dconnolly's messages when I dug into this it seemed to be caused by our panic = abort configuration. I was able to fix it by moving the client drop guard into the panic handler and calling close after reporting a panic.

@Swatinem
Copy link
Member

Ah that is indeed interesting, thanks a lot for investigating this!

And sure thing, when panics abort, the client won’t flush its queue. I wanted to add a flush hook to the transports as well, which we could use unconditionally in the panic handler.
I wouldn’t want to close the client completely in the hook, not until cfg(panic = "abort") is stabilized.

@ErichDonGubler
Copy link
Contributor

@WhyNotHugo: Was your configuration with panic = "abort", or panic = "unwind"? This same question could probably be usefully answered by @aramperes and @mattbeedle too -- if there's a pattern there, then that seems like a lot less risk for upgrading from 0.18 in the latter case of panic = "unwind".

@aramperes
Copy link
Contributor

Another thing I noticed is that the actix-web example doesn't work unless RUST_BACKTRACE is set to 1 (which defaults to 0).

@WhyNotHugo
Copy link
Author

@WhyNotHugo: Was your configuration with panic = "abort", or panic = "unwind"?

I have no explicit setting. I'm using 0.19 though (e.g.: not <= 0.18).

@ErichDonGubler
Copy link
Contributor

@WhyNotHugo: Okay, so, let me back up a bit; what I'm trying to figure out is if there are any patterns governing breakage going from 0.18 to 0.19 specifically. If you're not explicitly settings panic = "abort", then your binaries in OP that are reproducing the problem are probably panic = "unwind" (the default). My remaining question is, then, is if 0.18 exhibits the same issue, if your OP code sample were to be adapted to it (taking care to migrate backwards, e.g., remembering to add register_panic_handler. If 0.18 has the same behavior as 0.19, then this is an issue that probably needs to be fixed, but doesn't hurt users upgrading from 0.18 specifically. If this is a regression when upgrading from 0.18 to 0.19, though, then that's good information I want. :)

@yaahc
Copy link

yaahc commented Jan 4, 2021

@ErichDonGubler panic = unwind is probably not the only way to trigger this. A double panic, calling std::process::exit, Arc cyles, or even just calling std::mem::leak could also prevent the necessary drop impls from running.

@Swatinem
Copy link
Member

I released 0.23.0 recently, which does an explicit flush in the panic handler and will thus work correctly in the panic = "abort" case.

@oren0e
Copy link

oren0e commented Nov 9, 2021

So what should we use now (on versions 0.19 and above) instead of register_panic_handler()? Is it enough to use sentry::init() only?

@Swatinem
Copy link
Member

Swatinem commented Nov 9, 2021

The panic integration is on by default, so sentry::init() should be enough.

@indoood
Copy link

indoood commented Oct 19, 2022

I think I'm having a problem that looks very similar to the originally reported issue here.

Redacted what I said, it was an issue with internal Sentry configuration in my org. Nothing that impacts other people. I'm fine!

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

No branches or pull requests