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

Proc-macro pipeline (issue #113) #127

Closed
wants to merge 7 commits into from

Conversation

taqtiqa-mark
Copy link
Contributor

@taqtiqa-mark taqtiqa-mark commented Mar 23, 2022

@taqtiqa-mark
Copy link
Contributor Author

taqtiqa-mark commented Apr 28, 2022

@andylokandy this is ready for review - only failing test across whole workspace is the test for issue #141.

As things stand now the legacy tests for the legacy code all pass.

Further work is likely just going to identify existing bugs, e.g. issue #141, so this needs to land before further progress.

@taqtiqa-mark taqtiqa-mark changed the title WIP: Proc-macro pipeline (issue #113) Proc-macro pipeline (issue #113) Apr 28, 2022
@andylokandy
Copy link
Collaborator

@taqtiqa-mark Thanks so much for your work! I'm just back from vacation so I'll review it in short. With just a glance, I notice you've committed the whole embassy into the code base, is it intended? If so, what is the reason?

@andylokandy andylokandy requested review from zhongzc and andylokandy and removed request for zhongzc May 7, 2022 12:24
@taqtiqa-mark
Copy link
Contributor Author

taqtiqa-mark commented May 9, 2022

I notice you've committed the whole embassy into the code base, is it intended? If so, what is the reason?

AFAIK, that is the Embassy use pattern - there is no crate, as yet. Also issue #131 gives some context.

@andylokandy
Copy link
Collaborator

andylokandy commented May 11, 2022

case trace::generate::tests::generate_1 failed and confirmed on my local machine

Left:
TokenStream [
    Ident {
        sym: fn,
    },
    Ident {
        sym: f,
    },
    Group {
        delimiter: Parenthesis,
        stream: TokenStream [],
    },
    Group {
        delimiter: Brace,
        stream: TokenStream [
            Ident {
                sym: let,
            },
            Ident {
                sym: __guard,
            },
            Punct {
                char: '=',
                spacing: Alone,
            },
            Ident {
                sym: minitrace,
            },
            Punct {
                char: ':',
                spacing: Joint,
            },
            Punct {
                char: ':',
                spacing: Alone,
            },
            Ident {
                sym: local,
            },
            Punct {
                char: ':',
                spacing: Joint,
            },
            Punct {
                char: ':',
                spacing: Alone,
            },
            Ident {
                sym: LocalSpan,
            },
            Punct {
                char: ':',
                spacing: Joint,
            },
            Punct {
                char: ':',
                spacing: Alone,
            },
            Ident {
                sym: enter_with_local_parent,
            },
            Group {
                delimiter: Parenthesis,
                stream: TokenStream [
                    Literal {
                        lit: "f",
                    },
                ],
            },
            Punct {
                char: ';',
                spacing: Alone,
            },
            Group {
                delimiter: Brace,
                stream: TokenStream [],
            },
        ],
    },
]

Right:
TokenStream [
    Ident {
        sym: fn,
    },
    Ident {
        sym: f,
    },
    Punct {
        char: '<',
        spacing: Alone,
    },
    Punct {
        char: '>',
        spacing: Alone,
    },
    Group {
        delimiter: Parenthesis,
        stream: TokenStream [],
    },
    Group {
        delimiter: Brace,
        stream: TokenStream [
            Ident {
                sym: let,
            },
            Ident {
                sym: __guard,
            },
            Punct {
                char: '=',
                spacing: Alone,
            },
            Ident {
                sym: minitrace,
            },
            Punct {
                char: ':',
                spacing: Joint,
            },
            Punct {
                char: ':',
                spacing: Alone,
            },
            Ident {
                sym: local,
            },
            Punct {
                char: ':',
                spacing: Joint,
            },
            Punct {
                char: ':',
                spacing: Alone,
            },
            Ident {
                sym: LocalSpan,
            },
            Punct {
                char: ':',
                spacing: Joint,
            },
            Punct {
                char: ':',
                spacing: Alone,
            },
            Ident {
                sym: enter_with_local_parent,
            },
            Group {
                delimiter: Parenthesis,
                stream: TokenStream [
                    Literal {
                        lit: "f",
                    },
                ],
            },
            Punct {
                char: ';',
                spacing: Alone,
            },
            Group {
                delimiter: Brace,
                stream: TokenStream [],
            },
        ],
    },
]

Diff:
TokenStream [
    Ident {
        sym: fn,
    },
    Ident {
        sym: f,
    },
    Punct {
        char: '<',
        spacing: Alone,
    },
    Punct {
        char: '>',
        spacing: Alone,
    },
    Group {
        delimiter: Parenthesis,
        stream: TokenStream [],
    },
    Group {
        delimiter: Brace,
        stream: TokenStream [
            Ident {
                sym: let,
            },
            Ident {
                sym: __guard,
            },
            Punct {
                char: '=',
                spacing: Alone,
            },
            Ident {
                sym: minitrace,
            },
            Punct {
                char: ':',
                spacing: Joint,
            },
            Punct {
                char: ':',
                spacing: Alone,
            },
            Ident {
                sym: local,
            },
            Punct {
                char: ':',
                spacing: Joint,
            },
            Punct {
                char: ':',
                spacing: Alone,
            },
            Ident {
                sym: LocalSpan,
            },
            Punct {
                char: ':',
                spacing: Joint,
            },
            Punct {
                char: ':',
                spacing: Alone,
            },
            Ident {
                sym: enter_with_local_parent,
            },
            Group {
                delimiter: Parenthesis,
                stream: TokenStream [
                    Literal {
                        lit: "f",
                    },
                ],
            },
            Punct {
                char: ';',
                spacing: Alone,
            },
            Group {
                delimiter: Brace,
                stream: TokenStream [],
            },
        ],
    },
]

thread 'trace::generate::tests::generate_1' panicked at 'text differs', minitrace-macro/src/trace/generate.rs:72:9
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

@taqtiqa-mark
Copy link
Contributor Author

taqtiqa-mark commented May 12, 2022

@andylokandy I'm in the middle of something else, but Q that comes to mind, is if that is this:

@andylokandy this is ready for review - only failing test across whole workspace is the test for issue #141.

If so and its a blocker a workaround could be to start a bugs folder and move this test there and mark it as #[should_panic]?

@andylokandy
Copy link
Collaborator

andylokandy commented May 13, 2022

Generally LGTM. I have two questions left:

  1. What do you want to do with the currently unused attribute like recursive?
  2. The generate step seems not worth an extra layer, can it be merged with lowering?

@taqtiqa-mark
Copy link
Contributor Author

I have two questions left:

  1. What do you want to do with the currently unused attribute like recursive?

I'd prefer to leave them in place until the related issues are resolved one way or another. For a couple of reasons:
a) I may not get back to this for a while and it'll save me getting my head back into the right "space"
b) The API is still in flux (pre 1.0) so I don't believe it is unreasonable to have evolving code in the repo - I'll add comments and cite the related issues tracking the relevant attribute.

  1. The generate step seems not worth an extra layer, can it be merged with lowering?

Hmm, it did at one point. But this relates to the above question. Again a couple to reasons to prefer leaving it where it is:
a) As/if other features land this will likely do more work.
b) Simple is good, hopefully we can keep it this way - I fear not.
c) Most importantly, lowering is still a red-hot mess, adding this there just makes it worse.

Hopefully some of the lowering logic can be finessed, likely into the analysis stage - we'll see. But even if we could get all stages to be as clean/simple as generate is now, I'd still argue the pipeline structure communicates immediately what is happening (ack lowering is still an exception to that).

I'll take another look tomorrow and push up anything that results - likely mostly clean up and comments I outlined above.

@andylokandy
Copy link
Collaborator

@zhongzc PTAL

@andylokandy
Copy link
Collaborator

@taqtiqa-mark Very much appreciate your work. I'd be happy to merge it once the CI is satisfied, and @zhongzc has no objection.

@taqtiqa-mark
Copy link
Contributor Author

OK, I'll ping @andylokandy and @zhongzc when the CI is green.

@taqtiqa-mark
Copy link
Contributor Author

@andylokandy
Copy link
Collaborator

Github action says "step cannot have both the uses and run keys"

@taqtiqa-mark
Copy link
Contributor Author

Github action says "step cannot have both the uses and run keys"

Yeah, I had to take a break. Embassy is nearly in place but the CI build may need a rethink, unless going back to deal with the root cause (throwing --all-features and --all-tagets everywhere) doesn't simplify things.

@taqtiqa-mark
Copy link
Contributor Author

I suspect that adding minicov to the build stack could be required.

@andylokandy
Copy link
Collaborator

@taqtiqa-mark Looks good to me, let's take a try.

@taqtiqa-mark
Copy link
Contributor Author

taqtiqa-mark commented Jul 19, 2023

@taqtiqa-mark Looks good to me, let's take a try.

Apologies for the misunderstanding. I thought this comment meant someone else was going to look into this: let's -> let us.

I may have some time in the next few weeks, but my instinct is to try remove the embassy changes and leave embedded out of scope - unless I can recall why it is needed. Which I can't right now.

Close issue tikv#113.

Signed-off-by: Mark Van de Vyver <[email protected]>
@taqtiqa-mark
Copy link
Contributor Author

taqtiqa-mark commented Sep 13, 2023

@andylokandy this should be ready for a fresh set of eyes. I'm going to nurse it through the CI guards, but that shouldn't block a code review of the substantive changes.

On my machine cargo test returned no errors. I was also able to get the initial integration test to pass in minitrace-tests crate - which is the new test harness mentioned in #136 and #137.

If this all lands we can start moving some of the integration tests and the helper scripts. For now it is sufficient.

Close issue tikv#113.

Signed-off-by: Mark Van de Vyver <[email protected]>
Signed-off-by: Mark Van de Vyver <[email protected]>
@taqtiqa-mark
Copy link
Contributor Author

case trace::generate::tests::generate_1 failed and confirmed on my local machine

I believe this is expected:

test trace::generate::tests::generate_1 - should panic ... ok

Signed-off-by: Mark Van de Vyver <[email protected]>
Signed-off-by: Mark Van de Vyver <[email protected]>
@coveralls
Copy link

Pull Request Test Coverage Report for Build 6168403705

  • 1902 of 2341 (81.25%) changed or added relevant lines in 30 files are covered.
  • 80 unchanged lines in 5 files lost coverage.
  • Overall coverage decreased (-6.9%) to 75.932%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/collector/id.rs 23 24 95.83%
src/util/object_pool.rs 53 54 98.15%
test-utilities/src/lib.rs 11 12 91.67%
src/util/mod.rs 26 28 92.86%
src/local/local_span_line.rs 126 129 97.67%
src/util/tree.rs 121 124 97.58%
minitrace-macro/src/trace/lower/quotable.rs 20 25 80.0%
src/local/local_collector.rs 83 88 94.32%
src/local/local_span.rs 59 64 92.19%
src/util/spsc.rs 32 37 86.49%
Files with Coverage Reduction New Missed Lines %
minitrace-macro/src/lib.rs 2 5.88%
minitrace-datadog/src/lib.rs 3 90.48%
minitrace-jaeger/src/lib.rs 4 70.31%
minitrace-jaeger/src/thrift.rs 17 26.17%
minitrace-opentelemetry/src/lib.rs 54 1.52%
Totals Coverage Status
Change from base Build 5963933512: -6.9%
Covered Lines: 2016
Relevant Lines: 2655

💛 - Coveralls

Signed-off-by: Mark Van de Vyver <[email protected]>
Signed-off-by: Mark Van de Vyver <[email protected]>
@taqtiqa-mark
Copy link
Contributor Author

@andylokandy I think if you re-trigger the coveralls action you'll find the coverage has not deteriorated - even with the suspension of the additional tests that are awaiting the merge of this PR.

As far as I can tell I can't move the CI build actions so may need yourself or someone similarly situated to help get these all green.

@andylokandy
Copy link
Collaborator

@taqtiqa-mark I mis-opened the browser tab and commented in a wrong place. lol

I found that most of the code changes were about moving ./minitrace/src to ./src. What are the reasons for doing this? PS: As far as I know, large multi-crate projects arrange the directory structure in the way as minitrace, such as tokio.

@taqtiqa-mark
Copy link
Contributor Author

taqtiqa-mark commented Sep 13, 2023

No worries, gave me time to try and recall the reasons - its been near 18 months... I believe the reason is #143 . I don't recall the fine detail, but I'd say to be able to have feature flags passed to the minitrace-tests crate. Or some such reason.

@andylokandy
Copy link
Collaborator

andylokandy commented Sep 17, 2023

I have reviewed the majority of the files, which is a time-consuming process due to their volume. Before we proceed and merge the PR, there are several issues that need addressing:

  1. The project organization change may not be necessary; hence, I plan to revert this aspect and run everything to identify potential problems.

  2. The proc-macro code appears outdated:

(a) From minitrace 0.5 onwards, the #[trace] macro no longer mimics async-trait, so CollectLifetime and its counterparts should be removed.

(b) There are numerous unused options such as #[trace(variables = xxx)] and #[trace(parent = xxx)]. Rather than reviewing each individually, I suggest maintaining an unchanged user interface for now. We can discuss changes to the proc-macro interface in a separate issue while keeping this PR focused on internal refactoring.

Consequently, I propose merging the 'macro-test' portion of this PR first as it's straightforward and independent. Although initially centered around macros. If you agree, I would like to directly edit this PR.

@taqtiqa-mark
Copy link
Contributor Author

If you agree, I would like to directly edit this PR.

Sure, but later I'll try persuade you of an easier approach. If I understand correctly the challenge will be keeping track of what came from where, so I think you're proposing this kind of workflow:

git cherry-pick -n <commit> # get my pr, but don't commit (-n = --no-commit)
git reset                   # unstage the changes from my cherry-picked commit
git add -p                  # make all your choices (add the changes you do want)
git commit                # make the commit!

This would preserve the record of my effort. If so, then I'm happy with that approach and will help as and where I can.

Nonetheless, I suggest a less painful approach might be:

Create a merge-113 based on my PR. Then to address

  1. The project organization change may not be necessary; hence, I plan to revert this aspect and run everything to identify potential problems.

True this reorg may be unnecessary. That is, maybe core team should have closed the related cargo issue? Would be good if we have evidence it is no longer an issue.

To test this easily you can just re-point 4-5 cargo references to the mintrace-old (you may have to update mintrace-old but I believe it was a git rename so it should have the current upstream code that is in minitrace). If not, it is a trivial cut an paste into that folder and then re-point a 4-5 cargo references to see what happens.

  1. The proc-macro code appears outdated:....

True. However the current test suite passes. So a cleaner, and less effort, approach is to merge the PR and then add tests that should pass but fail. Then alter the code until they pass. This PR is not a release proposal and there is not reason anyone would expect master to be pristine.

  1. .... We can discuss changes to the proc-macro interface in a separate issue while keeping this PR focused on internal refactoring.

Some of this may also be moot. Several of these were proposed near 18 months ago. #135 has been closed. Similarly #138 was also closed.

Again here the most efficient way to address this is to write tests and remove/add code to make these green.

Ack this is a chicken and egg problem - the test infrastructure wasn't there previously so it was hard to do.

Any way, so we avoid the miscommunication we had earlier.

Please confirm you are now taking up this PR to the point it is rejected or merged?

@andylokandy
Copy link
Collaborator

andylokandy commented Sep 18, 2023

Please confirm you are now taking up this PR to the point it is rejected or merged?

Correct.

And here is my reminder, to avoid miscommunication:

  1. I will modify this pull request by incorporating additional commits (without force push).
  2. The new test cases and testing infrastructure will be merged following some necessary cleanups.
  3. Pick the up-to-date part of the proc-macro changes, like the pipeline structure.

@taqtiqa-mark
Copy link
Contributor Author

Thanks for the patience, ping me if you have any questions - hopefully I can remember what I was thinking :)

@taqtiqa-mark
Copy link
Contributor Author

@andylokandy is there anything I can do here? I don't recall seeing all these merge conflicts when I updated the PR. Let me know.

@andylokandy
Copy link
Collaborator

Sorry for the delay. I've being working on merging this pr but there are some reasons why I haven't push the update here. 1. since 0.5.0, the proc macro has been significantly simplified (no more lifetime related things), therefore the pipeline seems too heavy for it. 2. The macro ui changes are either unimplemented or identifying problems that has been resloved in other way since 0.5.0. 3. The ui tests cases are already merged by your former PRs. I'm very sorry, based on the current situation, this PR can not be merged.

@taqtiqa-mark
Copy link
Contributor Author

based on the current situation, this PR can not be merged.

No worries. Thanks for the update.

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.

3 participants