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

ObserverExceptionHandler for non async observers #29429

Closed
Postremus opened this issue Nov 23, 2022 · 6 comments · Fixed by #29445
Closed

ObserverExceptionHandler for non async observers #29429

Postremus opened this issue Nov 23, 2022 · 6 comments · Fixed by #29445
Assignees
Labels
area/arc Issue related to ARC (dependency injection) kind/enhancement New feature or request
Milestone

Comments

@Postremus
Copy link
Member

Postremus commented Nov 23, 2022

Description

I am facing a production issue right now, where all my log says is:

Failure occurred while notifying a transational Observer [method=observerMethod()] for event of type CustomType
- please enable debug logging to see the full stack trace

(I removed the FQCN, and replaced them with placeholders).

The above log is obiously not enough to figure out what the original exception was. Was it a TimeoutException while publishing to RabbitMQ, or was it a LazyInitException from some hibernate entity? The range of possibilities is simply to wide figure it out with this limited information.

I am not going to activate debug logging on the production system every time I face this issue. I need a solution which I configure once, and it just works.
Also, how it is implemented right now, this would lead to 2 lines logged. One error (the one above), and one debug.
The debug logs are not going into our usual monitoring solution - I would need to see the logs on the prod system.

This is where the logging is happening:

} catch (Exception e) {
// swallow exception and log errors for every problematic OM
LOG.errorf(
"Failure occurred while notifying a transational %s for event of type %s \n- please enable debug logging to see the full stack trace",
observerMethod, eventContext.getMetadata().getType().getTypeName());
LOG.debugf(e, "Failure occurred while notifying a transational %s for event of type %s",
observerMethod, eventContext.getMetadata().getType().getTypeName());
}

For Async Observers, I found AsyncObserverExceptionHandler, which allows to modify how their exceptions are logged:
https://quarkus.io/guides/cdi-reference#exceptions-thrown-by-an-asynchronous-observer-method

For normal Observers, can either:

  • the exception always be logged. Because what is someone going to do without it?
  • or an ObserverExceptionHandler be added, which would allow me to configure logging to my needs?

Implementation ideas

No response

@Postremus Postremus added the kind/enhancement New feature or request label Nov 23, 2022
@Postremus Postremus added area/arc Issue related to ARC (dependency injection) and removed triage/needs-triage labels Nov 23, 2022
@geoand
Copy link
Contributor

geoand commented Nov 23, 2022

cc @mkouba @manovotn

@manovotn
Copy link
Contributor

Standard CDI sync observers throw an exception and show you the track trace.
What you're seeing is only linked to transactional observers - I'll see if I can spot why we chose this approach in the first place...

@manovotn
Copy link
Contributor

The change comes from this PR - #23944
Notably, the behavior is also identical to that of Weld (compatible CDI impl).

Although it is not clear to me why we decided to do this for transactional observers instead of keeping it in line with how we treat other sync observers. @mkouba do you recall?

@mkouba
Copy link
Contributor

mkouba commented Nov 23, 2022

I don't remember but we should at least log the exception cause as Weld does.

@manovotn
Copy link
Contributor

Looking some more into the original issue, I think the reason might have been to reduce the noise from observer methods that can (deliberately?) sometimes throw an exception.
It looks like @famod and @gsmet both commented in favor of that, see #23944.

I don't remember but we should at least log the exception cause as Weld does.

I agree. I wouldn't even mind showing the whole error but due to the above, I don't think I'll get by with that :)
Anyhow, I'll send a PR that at least improves it.

@manovotn manovotn self-assigned this Nov 23, 2022
@mkouba
Copy link
Contributor

mkouba commented Nov 23, 2022

I agree. I wouldn't even mind showing the whole error but due to the above, I don't think I'll get by with that :)

The root cause class and exception message should be enough.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/arc Issue related to ARC (dependency injection) kind/enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants