wasmtime / issue #5194 offer function-level control over ... · git-wasmtime

Stream: git-wasmtime

Topic: wasmtime / issue #5194 offer function-level control over ...

Wasmtime GitHub notifications bot (Nov 03 2022 at 21:43):

github-actions[bot] commented on issue #5194:

Subscribe to Label Action

cc @kubkon

<details>
This issue or pull request has been labeled: "wasi"

Thus the following users have been cc'd because of the following labels:

kubkon: wasi

To subscribe or unsubscribe from this label, edit the <code>.github/subscribe-to-label.json</code> configuration file.

Learn more.
</details>

Wasmtime GitHub notifications bot (Nov 03 2022 at 22:38):

pchickey commented on issue #5194:

Thanks. I'd rather not break the current use of tracing: true | false in the macro if we can avoid it. For suppressing it on certain functions, could you add an optional suffix like, perhaps, tracing: true except { module::func, module2::func2?

Wasmtime GitHub notifications bot (Nov 03 2022 at 22:38):

pchickey edited a comment on issue #5194:

Thanks. I'd rather not break the current use of tracing: true | false in the macro if we can avoid it. For suppressing it on certain functions, could you add an optional suffix like, perhaps, tracing: true except { module::func, module2::func2 }?

Wasmtime GitHub notifications bot (Nov 03 2022 at 23:11):

joeshaw commented on issue #5194:

Thanks for the feedback, I'll look into it. My worry with except is that you might expect something different if you did tracing: false except { module::func } but there's probably another word we can use.

Wasmtime GitHub notifications bot (Nov 03 2022 at 23:23):

pchickey commented on issue #5194:

It should probably give an error if used in the false case? exclude is another word we could use.

Wasmtime GitHub notifications bot (Nov 04 2022 at 02:18):

alexcrichton commented on issue #5194:

Perhaps:
tracing: true, //  all functions

// ... or ...

tracing: false, // no functions

// ... or ...

tracing: ["..."], // only these functions
Would that work for your use case? I agree we'd ideally avoid various directions of lists here if we could. One possibility would be to support globs as well if families of functions to trace all have similar names. I suppose this could even go so far as to have:
tracing: ["trace-me", "!this-is-not-trace"],
where a ! in front disables tracing for that function (or any other matching globs)

Wasmtime GitHub notifications bot (Nov 04 2022 at 14:51):

joeshaw commented on issue #5194:

@alexcrichton For my use case, I am looking to suppress tracing from a function that carries sensitive information, so negation is the only thing that'd work.

Taking a step back: is there a strong reason to have the tracing config be highly customizable? I think I have a compelling reason for _disabling_ tracing on a particular function but is it important to be able to turn it on for just a few functions?

I'm also hesitant to diverge too much from syntax that already exists. async allows either a catchall (async: *) or a list of identifiers in braces (async: { module::func1, module::func2 }).

Wasmtime GitHub notifications bot (Nov 04 2022 at 16:46):

alexcrichton commented on issue #5194:

Keeping consistent with async seems reasonable, but I think that it's fine to change the syntaxes for both at the same time. I would agree that there's probably not much of a use case for "just this function". Perhaps:
tracing: true,
tracing: false,
tracing_skip: [ ... ]
and that may work?

Wasmtime GitHub notifications bot (Nov 04 2022 at 17:44):

joeshaw commented on issue #5194:

I pushed a new set of commits that implements what @pchickey suggested (but with disable_for instead of except -- but happy to change this back).

Example usage:
wiggle::from_witx!({
    tracing: true disable_for {
        module1::func,
        module2::another_func,
    },
    witx: ["..."],
});

Wasmtime GitHub notifications bot (Nov 04 2022 at 17:49):

joeshaw edited a comment on issue #5194:

I pushed a new set of commits that implements what @pchickey suggested (but with disable_for instead of except because I think it flows better even if you change true to false -- but happy to change this back).

Example usage:
wiggle::from_witx!({
    tracing: true disable_for {
        module1::func,
        module2::another_func,
    },
    witx: ["..."],
});

Wasmtime GitHub notifications bot (Nov 04 2022 at 17:50):

joeshaw edited a comment on issue #5194:

I pushed a new set of commits that implements what @pchickey suggested (but with disable_for instead of except because I think it flows better even if you change true to false -- but happy to change this back).

Example usage:
wiggle::from_witx!({
    tracing: true disable_for {
        module1::func,
        module2::another_func,
    },
    witx: ["..."],
});
I'm also happy to switch this to be a separate argument altogether if that's preferred.

Wasmtime GitHub notifications bot (Nov 04 2022 at 17:51):

joeshaw edited a comment on issue #5194:

I pushed a new set of commits that implements what @pchickey suggested (but with disable_for instead of except because I think it flows better even if you change true to false -- but happy to change this back).

Example usage:
wiggle::from_witx!({
    tracing: true disable_for {
        module1::func,
        module2::another_func,
    },
    witx: ["..."],
});
I'm also happy to switch this to be a separate argument altogether (@alexcrichton's tracing_skip suggestion) if that's preferred.

Wasmtime GitHub notifications bot (Nov 04 2022 at 19:49):

alexcrichton commented on issue #5194:

Seems reasonable to me :+1: but I defer to @pchickey for the final approval

Wasmtime GitHub notifications bot (Nov 05 2022 at 18:31):

pchickey commented on issue #5194:

Thanks @joeshaw, this is good work.

Last updated: Nov 22 2024 at 17:03 UTC