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

WIP: Pass Trace transparently through withError #3358

Draft
wants to merge 25 commits into
base: arrow-2
Choose a base branch
from

Conversation

kyay10
Copy link
Collaborator

@kyay10 kyay10 commented Jan 23, 2024

This is achieved by sealing Raise to check DefaultRaise.isTraced.

@kyay10 kyay10 self-assigned this Jan 23, 2024
@kyay10 kyay10 added the 2.0.0 Tickets / PRs belonging to Arrow 2.0 label Jan 23, 2024
@kyay10 kyay10 linked an issue Jan 23, 2024 that may be closed by this pull request
Copy link
Contributor

github-actions bot commented Jan 23, 2024

@kyay10
Copy link
Collaborator Author

kyay10 commented Jan 23, 2024

Turning Raise into a sealed interface feels like a very hard change to justify. I think there's some use cases opened up here, though, like accessing isTraced.

The implementors of Raise should be very few and far in between, and both provided APIs (TransformingRaise and RaiseWrapper) should cover all use cases (one can throw an exception inside transform if they really want to override raise's behaviour).

RaiseWrapper can be very easily deprecated as well when context receivers come around, since its implementors are ones where migrating to contexts is very easy.
TransformingRaise might stick around for longer because it makes certain implementations of Raise have somewhat less allocations, and hence might be preferable (e.g. in SingletonRaise and RaiseAccumulate). I've ended up keeping it around in arrow-context for that reason, although I can see an argument that it's rather pointless to keep it and that, instead, one should just use withError.

@kyay10
Copy link
Collaborator Author

kyay10 commented Jan 24, 2024

Requesting reviews just to get some thoughts on this. I'm not fully convinced that this PR is the right approach, but It's hard to see how else to conditionally trace based on whether the caller is in a traced context or not.

Food for thought: with this change and #3349, maybe it's time to redesign traced?

@@ -143,7 +143,7 @@ public annotation class RaiseDSL
* <!--- KNIT example-raise-dsl-04.kt -->
* <!--- TEST lines.isEmpty() -->
*/
public interface Raise<in Error> {
public sealed interface Raise<in Error> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Honestly, for me this is a no-go, we really want to keep this one open for extension (we even talk about that in the docs).

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that I'm keeping it open by instead providing RaiseWrapper and TransformingRaise. Those 2 are the only use cases that I can observe. Most people will use RaiseWrapper if they're just defining an intersection type (and we can deprecate it when contexts are stable). TransformingRaise is another pattern where one wants to change the error type right then and there (used in RaiseAccumulate and SingletonRaise), and it's slightly more performance than withError.

Instead of this change, would you prefer we force implementors to provide a val underlyingRaise: Raise<*> or something along those lines? I didn't want to go through that approach because one can do val underlyingRaise get() = this which I heavily dislike

Copy link
Member

@nomisRev nomisRev Jan 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with @serras. I understand it's still open for extension, but not in a straightforward way.

Did not yet have the time to properly dive into this, but perhaps we can find a different solution. Perhaps going the other-way around might be better. Something like CoroutineStackFrame which you can optionally implement, if you need behavior like this one.

We can dynamically check if a Raise interface implements RaiseFrame, or similar and then we'd get access to additional properties. That would probably even be possible to add in 1.x

No idea if any of this would work, just spitting some ideas ☺️ Will make some time asap to look into this issue.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've head of teams using CoroutineContext to pass this information, but unfortunately we don't have suspend in our raise function.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about introducing some form of interface RaiseWrapper<Error>: Raise<Error> { val underlyingRaise: Raise<*> } and encouraging users to gradually use it? We can support this feature then for those implementors. Alternatively, we can add an val underlyingRaise into Raise directly, with a default impl of returning this, although this would be a 2.0 feature.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe I'm also getting distracted by thinking too far into the future. With contexts, we don't want "delegating" implementors of Raise, because they can always be replaced by composing Raise and the class in question. Instead, we want "transforming" implementors that actually do something interesting with the error value. Maybe that, as a plan, is unnecessarily ambitious. I'll experiment here with simplifying this, focusing on requiring as little migration as possible.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.0.0 Tickets / PRs belonging to Arrow 2.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

["Request"] better stack traces when tracing withError
3 participants