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

trace macro #413

Merged
merged 8 commits into from
May 28, 2024
Merged

trace macro #413

merged 8 commits into from
May 28, 2024

Conversation

fwbrasil
Copy link
Collaborator

I'm working on #213. This PR introduces a macro to generate Traces including a snippet of the method call. The macro isn't very robust because I couldn't find a way to obtain the snippet via the macro APIs. Position.ofMacroExpansion returns the position of the Trace macro expansion, not the method call. The implementation uses a heuristic to extract the call directly from the source code string. Since the snippet is informative, I think it's a reasonable risk.

@fwbrasil fwbrasil mentioned this pull request May 24, 2024
@fwbrasil
Copy link
Collaborator Author

fwbrasil commented May 24, 2024


"two params" in {
def test(a: Int, b: String)(using t: Trace) = t.snippet
assert(test(1, "hello") == "test(1, \"hello\")")
Copy link
Collaborator

Choose a reason for hiding this comment

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

might be easier to maintain/read tests if we use triple quotes.

object Test:
infix def test(v: Int)(using t: Trace) = t.snippet

assert((Test test 42) == "Test test 42")
Copy link
Collaborator

Choose a reason for hiding this comment

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

How would this be serialized differently?

Maybe an extension infix method might be a good test as well.

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 would this be serialized differently?

Note that the test is pending. The macro is currently generating an empty string for this scenario.

Maybe an extension infix method might be a good test as well.

I'll add a pending test for it as well.

README.md Outdated
@@ -1076,7 +1077,7 @@ def example(db: Database): Int < IOs =
// Another 'Cut' implementation
val countTimesTen =
new Cut[Database, Int, IOs] {
def apply[S](v: Database < S)(f: Database => Int < IOs) =
def apply[S](v: Database < S)(f: Database => Int < IOs)(using Trace) =
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why does the readme need to mention Trace at all?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I thought it'd be nice to ensure aspects propagate traces but, rethinking it now, it doesn't seem to make sense. We do want to show the trace based on the user's provided implementation for apply.

class TraceTest extends KyoTest:

"show" in {
def test(i: Int)(using t: Trace) = t.show
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you add a .show with several layers of nesting? I would like to see how the overall message appears.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

do you think it's still necessary after the clarifications in private? If not, can you provide an example of what you have in mind?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Nope

README.md Outdated
@@ -1,4 +1,4 @@
### Please visit https://getkyo.io for an indexed version of this documentation.
1119### Please visit https://getkyo.io for an indexed version of this documentation.
Copy link
Collaborator

Choose a reason for hiding this comment

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

accident?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yep!

@fwbrasil
Copy link
Collaborator Author

I should have clarified something: this change is only an initial step to address #213. The trace information is stored in all Kyo computations, but we still need to change the places where effects are evaluated (core, IOs, IOTask, ZIOs) to accumulate the traces and provide a mechanism similar to stack traces. I might first explore a way to avoid custom effect handling in all these different places. Ideally, they shouldn't rely on internal core classes.

Meanwhile, once this change is merged, we can already start showing frame information for example in the scheduler top. I imagine we'll find some other use cases as well.

@hearnadam
Copy link
Collaborator

Changes LGTM, do you still want to hold off on merging for another release?

@fwbrasil
Copy link
Collaborator Author

@hearnadam I'm planning to merge this once the build passes since we've made the release already. Maintaining the branch generates extra work and I want to work on follow ups on top of this change.

@fwbrasil fwbrasil merged commit 203a729 into main May 28, 2024
3 checks passed
@fwbrasil fwbrasil deleted the trace-macro branch May 28, 2024 20:19
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

Successfully merging this pull request may close these issues.

None yet

2 participants