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

Allow for recording an error in spans without throwables #939

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ideskov
Copy link

@ideskov ideskov commented Jan 7, 2025

Hey! It would be really nice if we could attach error events to spans without having to pass Throwables around. This is an attempt to cater for a more functional style of programming where exceptions are avoided and errors are part of methods' signatures.

I'm unsure whether this.delegate.tag("error", message); is enough for the Brave bridge. I didn't find many options in the native API.

Thanks for considering this proposal!

@jonatan-ivanov
Copy link
Member

jonatan-ivanov commented Jan 7, 2025

Thank you for the PR! Do you have a real use-case for this? If so, could you please show us what you are trying to do?

What concerns me that both OTel and Brave have their own mechanisms to signal an error which is possible through passing a Throwable and this change bypasses those mechanisms so it could be an endless source of issues since we don't know what the methods that are not called are doing.

If you want the "normal" behavior of these libraries I think you can do something like this:

span.error(new MyCustomError(message));

You don't need to throw the exception, it can be used as a wrapper around the error message and the stacktrace (and other details you want to attach).

If you want just an event (won't fail the span):

span.event("error: oops!");

@shakuzen @marcingrzejszczak What do you think? Has something like this requested before?

@ideskov
Copy link
Author

ideskov commented Jan 8, 2025

Do you have a real use-case for this?

Sure, our code base is mainly in Kotlin utilising this functional companion library arrow-kt. In essence, we don't work with exceptions at all. Instead, we define possible errors as part of function signatures utilising the Either type, e.g.:

fun createPayment(request: PaymentRequest): Either<PaymentError, Payment> { ... }

So what this means is if we want to record an error in a span, we need to artificially create an exception, like so:

span.error(RuntimeException("invalid payment"))

leaving us with a stacktrace in the span which is completely useless and only creates noise in Honeycomb.

Ideally, what we want is to attach an event to the span with a helpful attribute indicating what the error was and set the span status to indicate that there's been an error. Setting the span status is significant as we don't want these to be sampled.

Edit: PaymentError in the example above isn't a Throwable. Usually that would be a sealed type extended by either objects or data classes.

@marcingrzejszczak
Copy link
Contributor

I think that setting an event + customizing OTel components to mark the status as error when an event with a given text was found would be the way to go for now.

I don't recall anyone asking for this really.

@ideskov
Copy link
Author

ideskov commented Jan 8, 2025

I think that setting an event + customizing OTel components to mark the status as error when an event with a given text was found would be the way to go for now.

Sorry, I'm confused. Recording an event + marking the span status as error is what the proposed change does. Does your statement mean that you're opposing the change and clients of micrometer should rather switch to using the native otel API?

@marcingrzejszczak
Copy link
Contributor

I think that setting an event + customizing OTel components to mark the status as error when an event with a given text was found would be the way to go for now.

I'm saying that this would be the way to go without this change. With this change we're adding new API and we're always trying to be cautious about that

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants