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

RFC proc-macro structure #113

Closed
taqtiqa-mark opened this issue Mar 15, 2022 · 7 comments
Closed

RFC proc-macro structure #113

taqtiqa-mark opened this issue Mar 15, 2022 · 7 comments

Comments

@taqtiqa-mark
Copy link
Contributor

taqtiqa-mark commented Mar 15, 2022

Related to issue #81
In the course of issue #112

I had a look at the proc-macro crate, with a view to fixing #112. Not being a proc-macro guru I'm a little intimidated/lost by the current structure.
The proposal is to, in the first instance restructure the code into the following pipeline:

mermaid-diagram-20220224191222

The file layout would be:

src
|-- lib.rs
`-- trace
    |-- analyze.rs
    |-- codegen.rs
    |-- lower.rs
    `-- parse.rs

New proc-macro-attributes, proc-macro-functions would be expected to follow the same convention.

The idea would be to write tests as the code is migrated.

A WIP example is in the issue-1 branch of tracing-attributes-http:

https://github.com/taqtiqa-mark/tracing-attributes-http/tree/issue-1/src

Thoughts?

@andylokandy
Copy link
Collaborator

andylokandy commented Mar 15, 2022

Thank you for bringing this up, the proposal looks petty neat to me. The current codebase is not intended to be this messy and mysterious... It's simply because I've put all ideas into it in one night and to prove they work, so the code structure was not seriously taken into consideration.

To leave some useful information for who is interested in refactoring the macro (I may do it months later if no one does it by that time), here is the original place where some of the codes are copied from:

@andylokandy
Copy link
Collaborator

andylokandy commented Mar 15, 2022

The reason why we've to elaborate async fn into fn -> impl Future is that, for an async fn foo(), we have to instrument the Span::enter_with_local_parent in the first call to foo(), but not in the poll() or .await, because it's the only chance that local parent is prenent in the context.

@taqtiqa-mark
Copy link
Contributor Author

I think to begin with it'll be baby steps:

  1. Restructure the code layout and introduce tests.
  2. Error reporting / UI
  3. Once some tests are in place would be to introduce a model, etc.

@breezewish
Copy link
Member

breezewish commented Mar 15, 2022

RFC is a generally great idea for such purpose. However with my current practice experience it may affect agility or hard to be followed until we find a good balance / standard about what should be tracked by a RFC.

I'm also thinking about a lighter way for summarize new changes which should be easier to be followed: always describe the change with more details in the issue + finalize ideas in the doc comment + verbose function comments. We should also have verbose comments for existing code bases.

What do you think?

@andylokandy
Copy link
Collaborator

andylokandy commented Mar 15, 2022

From my personal experience in open source projects, for this kind of small project, I'll directly open a pull request to bootstrap the discussion. Although there are risks of going into a completely wrong direction and wasting time, I found it rather efficient to drive me and the reviewers to a better solution even if my first try is bad.

@taqtiqa-mark
Copy link
Contributor Author

Agreed. The main risk here is rejecting the whole pipeline idea. I don't propose the RFC be more detailed than what it is now, e.g. the Model is undefined/unspecified - it'll emerge, as you say small PR by small PR.

The goal is to keep the proc-macro working the whole time. I'll try do this by changing the code once it is covered by a test case, so related to #81.

So I believe we are agreed, this will not land with a big-bang single PR.
Rather smaller PRs will indicate how they are heading in the direction set out in this RFC.
Correct?

taqtiqa-mark added a commit to taqtiqa-mark/utrace-rust that referenced this issue Mar 20, 2022
This test reflects the current state of play.

Migrating to the pipeline set out in issue tikv#113 will enable this
to be replaced by the more informative text and error location:

error: this attribute takes one argument

         = help: use

 --> tests/ui/has-arguments.rs:3:1
  |
3 | #[trace(a, b)]
  | ^^^^^^^^^^^^^^^^^^^^
  |
  = note: this error originates in the attribute macro
    (in Nightly builds, run with -Z macro-backtrace for more info)

Signed-off-by: Mark Van de Vyver <[email protected]>
taqtiqa-mark added a commit to taqtiqa-mark/utrace-rust that referenced this issue Mar 21, 2022
Address issue tikv#81.

These tests reflect the current state of play.

They ensure migrating to the pipeline set out in issue tikv#113,
does not break existing functionality.

Signed-off-by: Mark Van de Vyver <[email protected]>
zhongzc pushed a commit that referenced this issue Mar 22, 2022
* Doc: Describe pipeline and test execution

Signed-off-by: Mark Van de Vyver <[email protected]>

* Add: Cargo configuration for UI test suite

Signed-off-by: Mark Van de Vyver <[email protected]>

* Add: Legacy UI test parameter inputs - case 1

This test reflects the current state of play.

Migrating to the pipeline set out in issue #113 will enable this
to be replaced by the more informative text and error location:

error: this attribute takes one argument

         = help: use

 --> tests/ui/has-arguments.rs:3:1
  |
3 | #[trace(a, b)]
  | ^^^^^^^^^^^^^^^^^^^^
  |
  = note: this error originates in the attribute macro
    (in Nightly builds, run with -Z macro-backtrace for more info)

Signed-off-by: Mark Van de Vyver <[email protected]>

* Add: Legacy UI test parameter inputs - case 2

Signed-off-by: Mark Van de Vyver <[email protected]>

* Add: Legacy UI test parameter inputs - case 3
Signed-off-by: Mark Van de Vyver <[email protected]>

* Add: Legacy UI test parameter inputs - case 4
Signed-off-by: Mark Van de Vyver <[email protected]>

* Evolve: Legacy UI test parameter inputs

Signed-off-by: Mark Van de Vyver <[email protected]>

* Fix: Legacy UI test parameter input result

Signed-off-by: Mark Van de Vyver <[email protected]>

* Add: Legacy UI test parameter inputs - case 5
Signed-off-by: Mark Van de Vyver <[email protected]>

* Add: Legacy UI test parameter inputs - case 6
Signed-off-by: Mark Van de Vyver <[email protected]>

* Add: Legacy UI test parameter inputs - case 7
Signed-off-by: Mark Van de Vyver <[email protected]>

* Add: Legacy UI test parameter inputs - case 8
Signed-off-by: Mark Van de Vyver <[email protected]>

* Add: Trace macro tests

Signed-off-by: Mark Van de Vyver <[email protected]>

* Add: Legacy UI test parameter inputs - case 9
Signed-off-by: Mark Van de Vyver <[email protected]>

* Add: Legacy UI test parameter inputs - case 10
Signed-off-by: Mark Van de Vyver <[email protected]>

* Add: Legacy UI test parameter inputs - case 11
Signed-off-by: Mark Van de Vyver <[email protected]>

* Add: Legacy UI test parameter inputs - case 12
Signed-off-by: Mark Van de Vyver <[email protected]>
taqtiqa-mark added a commit to taqtiqa-mark/utrace-rust that referenced this issue Mar 23, 2022
This test reflects the current state of play.

Migrating to the pipeline set out in issue tikv#113 will enable this
to be replaced by the more informative text and error location:

error: this attribute takes one argument

         = help: use

 --> tests/ui/has-arguments.rs:3:1
  |
3 | #[trace(a, b)]
  | ^^^^^^^^^^^^^^^^^^^^
  |
  = note: this error originates in the attribute macro
    (in Nightly builds, run with -Z macro-backtrace for more info)

Signed-off-by: Mark Van de Vyver <[email protected]>
taqtiqa-mark added a commit to taqtiqa-mark/utrace-rust that referenced this issue Sep 13, 2023
Close issue tikv#113.

Signed-off-by: Mark Van de Vyver <[email protected]>
taqtiqa-mark added a commit to taqtiqa-mark/utrace-rust that referenced this issue Sep 13, 2023
Close issue tikv#113.

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

Voided by PR #127 rejection.

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

No branches or pull requests

3 participants