Proc-macro pipeline (issue #113)

[taqtiqa-mark]

• [x] Implement Parse stage
• [x] ~~Implement Validate stage.~~ See issue #142
• [x] Implement Model stage
• [x] Implement Lower stage
• [x] Implement Generate stage
• [x] test-utilities crate for test helper functions
• [x] mintrace-tests crate for integration tests
  • [x] Integrate mintrace-tests crate
  • [x] Migrate mintrace-macro tests/spans to mintrace-tests:
    • Tracking issue #136
  • [x] Migrate mintrace-macro tests/expand to mintrace-tests:
    • Tracking issue #137
• [x] Create UI integration tests of legacy implementation logic
• [x] Create span integration tests of legacy implementation logic
• [x] Create expansion integration tests of legacy implementation logic
• [x] Remove proc-macro-error usage, per @Yandros comment
  • [x] Make IDE friendly
• [x] Cleanup
• [x] Rebase on master

[taqtiqa-mark]

@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.

[andylokandy]

@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?

[taqtiqa-mark]

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]

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]

@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 https://github.com/tikv/minitrace-rust/issues/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]

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]

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.

2. 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]

@zhongzc PTAL

[andylokandy]

@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]

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

[taqtiqa-mark]

Note to self: https://github.com/nicolas-van/rust-cross-compile-example/blob/main/.github/workflows/rust.yml

[andylokandy]

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

[taqtiqa-mark]

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]

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

[andylokandy]

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

[taqtiqa-mark]

@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.

[andylokandy]

@taqtiqa-mark, the macro has updated the output for async functions since version 0.5.0. Please adjust your pull request accordingly.

[taqtiqa-mark]

@andylokandy I've merged the upstream changes, and am now working on integrating the side effects. It has been a while since I used minitrace and I don't have time to get back up to speed now, and for the forseeable future.... so would like your help by indicating the idiomatic way to rewrite the following test case - there seem to have been substantial changes around the use of collectors:

extern crate alloc;
use minitrace::trace;
use minitrace::prelude::*;
use test_utilities::*;

// With default (`enter_on_poll = false`), `async` functions construct
// `Span` that is thread safe.

// With no block expression the child span is silently omitted.
// Reference:
// - https://github.com/tikv/minitrace-rust/issues/125
// - https://github.com/tikv/minitrace-rust/issues/126

async fn test_async(a: u32) -> u32 {
    a
}

#[trace]
fn test_sync(a: u32) -> u32 {
    a
}

#[tokio::main]
async fn main() {
    let (root, collector) = minitrace::Span::root("root");
    let _child_span = root.set_local_parent();
    let mut handles = vec![];
    handles.push(tokio::spawn(test_async(1)));
    test_sync(2);

    futures::future::join_all(handles).await;
    drop(root);
    let records: Vec<minitrace::collector::SpanRecord> =
        futures::executor::block_on(collector.collect());
    let _expected = r#"[
    SpanRecord {
        id: 1,
        parent_id: 0,
        begin_unix_time_ns: \d+,
        duration_ns: \d+,
        event: "root",
        properties: [],
    },
]"#;
    let _actual = normalize_spans(records);
    assert_eq_text!(_expected, &_actual);
}

[taqtiqa-mark]

@andylokandy and @zhongzc understand you're busy, and the delay in this was due to the miscommunication above. Nonetheless, I do have a limited amount of personal free time I can give to this. Appreciate your prompt attention.

I also understand if things have evolved to a point you don't want to proceed with this approach. Please let me know if that is the case.

For this example I get a future related error:

Setup
Running 1 macro expansion tests
....
integration/tests/defaults/no-be-no-drop-local.expanded.rs
error
┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈
error[E0277]: `LocalSpans` is not a future
   --> integration/tests/defaults/no-be-no-drop-local.expanded.rs:24:9
    |
23  |     let records: Vec<minitrace::collector::SpanRecord> = futures::executor::block_on(
    |                                                          --------------------------- required by a bound introduced by this call
24  |         collector.collect(),
    |         ^^^^^^^^^^^^^^^^^^^ `LocalSpans` is not a future
    |
    = help: the trait `futures::Future` is not implemented for `LocalSpans`
    = note: LocalSpans must be a future or must implement `IntoFuture` to be awaited
note: required by a bound in `block_on`
   --> /home/hedge/.local/share/cargo/registry/src/index.crates.io-6f17d22bba15001f/futures-executor-0.3.21/src/local_pool.rs:313:20
    |
313 | pub fn block_on<F: Future>(f: F) -> F::Output {
    |                    ^^^^^^ required by this bound in `block_on`

error[E0277]: `LocalSpans` is not a future
  --> integration/tests/defaults/no-be-no-drop-local.expanded.rs:23:58
   |
23 |       let records: Vec<minitrace::collector::SpanRecord> = futures::executor::block_on(
   |  __________________________________________________________^
24 | |         collector.collect(),
25 | |     );
   | |_____^ `LocalSpans` is not a future
   |
   = help: the trait `futures::Future` is not implemented for `LocalSpans`
   = note: LocalSpans must be a future or must implement `IntoFuture` to be awaited

error[E0277]: `LocalSpans` is not a future
  --> integration/tests/defaults/no-be-no-drop-local.expanded.rs:23:58
   |
23 |     let records: Vec<minitrace::collector::SpanRecord> = futures::executor::block_on(
   |                                                          ^^^^^^^^^^^^^^^^^^^^^^^^^^^ `LocalSpans` is not a future
   |
   = help: the trait `futures::Future` is not implemented for `LocalSpans`
   = note: LocalSpans must be a future or must implement `IntoFuture` to be awaited

[andylokandy]

@taqtiqa-mark Thank you for continuing to work together to improve this project, thank you very much. I apologize for the late reply. This is because we have been focusing on other projects recently, but you don’t have to worry. If you have any questions, you can ask, and we will reply as soon as possible.

The biggest change in minitrace 0.5 is that the previously manually operated collector has been changed to a global automatic global reporter. Accordingly, LocalCollector::collect and Span::root interfaces no longer return Future, span will automatically be passed to the global reporter when dropped, so unit tests also need to be changed. Minitrace 0.5 provides a hidden TestReporter to help write unit tests, you can refer to the existing test set of minitrace existing test set for usage methods.

[taqtiqa-mark]

@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.

[taqtiqa-mark]

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

[coveralls]

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%
<!--	Total:	1902	2341

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%
<!--	Total:	80

Totals
Change from base Build 5963933512:	-6.9%
Covered Lines:	2016
Relevant Lines:	2655

---

💛 - Coveralls

[taqtiqa-mark]

@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]

@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]

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]

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]

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.

2. 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.

2. .... 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]

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]

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