Stream: git-wasmtime

Topic: wasmtime / PR #1796 wiggle: switch logging to use `tracin...

Wasmtime GitHub notifications bot (Jun 03 2020 at 17:57):

pchickey edited PR #1796 from pch/wiggle_tracing to master:

Wiggle generated code now emits code that uses tracing for logging.

We use tracing by default at Fastly, so I'd like to use it in wiggle and wasi-common without adapters if possible. Tracing has a couple of advantages over log that make it well suited here:

• The concept of spans, which are natural to use in wiggle-generated functions where we want to log the arguments of a function, then later when an inner func returns we want to log the return values. the argument values and return value s are part of the same span, which means the backend can automatically correlate them, rather than have to associate them by searching on some common part of the text

• (stable) support for structured fields (in log this is noted as unstable and called kv support), which also naturally matches to the names of arguments / return values we already have in witx, and keeps that structure around longer so that it can be e.g. encoded in json by the backend that sends it to some logging endpoint that understands json

• tracing statements that are disabled (marked as uninteresting, essentially) by the backend are very low overhead - its one atomic load to determine whether a hook is disabled.

<!--

Please ensure that the following steps are all taken care of before submitting
the PR.

• [ ] This has been discussed in issue #..., or if not, please tell us why
  here.

• [ ] A short description of what this does, why it is needed; if the
  description becomes long, the matter should probably be discussed in an issue
  first.

• [ ] This PR contains test cases, if meaningful.

• [ ] A reviewer from the core maintainer team has been assigned for this PR.
  If you don't know who could review this, please indicate so. The list of
  suggested reviewers on the right can help you.

Please ensure all communication adheres to the code of conduct.
-->

Wasmtime GitHub notifications bot (Jun 03 2020 at 21:30):

alexcrichton submitted PR Review.

Wasmtime GitHub notifications bot (Jun 03 2020 at 21:30):

alexcrichton submitted PR Review.

Wasmtime GitHub notifications bot (Jun 03 2020 at 21:30):

alexcrichton created PR Review Comment:

FWIW this is a pretty hefty dependency list. I think were ok on spidermonkey for this PR itself because this only affects wasi-common which spidermonkey isn't vendoring yet. In that sense it'll be an issue if we add tracing more deeply in cranelift/wasmtime, but for now it should be ok.

There's no way though to depend on only the macros?

(I know nothing about tracing)

Wasmtime GitHub notifications bot (Jun 03 2020 at 21:30):

alexcrichton created PR Review Comment:

I think this could perhaps be pub extern crate tracing;?

Wasmtime GitHub notifications bot (Jun 03 2020 at 22:09):

hawkw created PR Review Comment:

tracing-subscriber is not required for the macros. It contains actual tracing::Subscriber implementations (like loggers in the log ecosystem), and is not part of the minimal facade layer.

I believe that this crate is just a dev-dependency, presumably for testing that the emitted traces work correctly or something?

Wasmtime GitHub notifications bot (Jun 03 2020 at 22:09):

hawkw submitted PR Review.

Wasmtime GitHub notifications bot (Jun 03 2020 at 22:11):

hawkw edited PR Review Comment.

Wasmtime GitHub notifications bot (Jun 03 2020 at 22:14):

sunfishcode submitted PR Review.

Wasmtime GitHub notifications bot (Jun 03 2020 at 22:14):

sunfishcode submitted PR Review.

Wasmtime GitHub notifications bot (Jun 03 2020 at 22:14):

sunfishcode created PR Review Comment:

Please add spaces before and after the '='

Wasmtime GitHub notifications bot (Jun 03 2020 at 22:16):

hawkw edited PR Review Comment.

Wasmtime GitHub notifications bot (Jun 03 2020 at 22:22):

hawkw submitted PR Review.

Wasmtime GitHub notifications bot (Jun 03 2020 at 22:22):

hawkw submitted PR Review.

Wasmtime GitHub notifications bot (Jun 03 2020 at 22:22):

hawkw created PR Review Comment:

Note that --- as in log --- the max level is _inclusive_, so this enables all spans/events with a level less than _or equal_ to TRACE. In order to enable DEBUG and lower, you would use tracing::Level::DEBUG here instead.

Wasmtime GitHub notifications bot (Jun 03 2020 at 22:35):

pchickey submitted PR Review.

Wasmtime GitHub notifications bot (Jun 03 2020 at 22:35):

pchickey created PR Review Comment:

Yes, tracing-subscriber gets pulled in because of the wiggle-test example. I'm OK with shipping this code without the example, which doesn't provide much of any assurance that things work in CI beyond compiling - I didn't work on writing an actual test.

So, if we want to save the project's Cargo.lock from pulling all of that stuff in, we can drop tracing-subscriber from the example (I'll probably leave it in as commented-out code, with a note that the dep tree is too heavy for this project) and use it to just demonstrate the log integration.

Wasmtime GitHub notifications bot (Jun 03 2020 at 22:36):

pchickey updated PR #1796 from pch/wiggle_tracing to master:

Wiggle generated code now emits code that uses tracing for logging.

We use tracing by default at Fastly, so I'd like to use it in wiggle and wasi-common without adapters if possible. Tracing has a couple of advantages over log that make it well suited here:

• The concept of spans, which are natural to use in wiggle-generated functions where we want to log the arguments of a function, then later when an inner func returns we want to log the return values. the argument values and return value s are part of the same span, which means the backend can automatically correlate them, rather than have to associate them by searching on some common part of the text

• (stable) support for structured fields (in log this is noted as unstable and called kv support), which also naturally matches to the names of arguments / return values we already have in witx, and keeps that structure around longer so that it can be e.g. encoded in json by the backend that sends it to some logging endpoint that understands json

• tracing statements that are disabled (marked as uninteresting, essentially) by the backend are very low overhead - its one atomic load to determine whether a hook is disabled.

<!--

Please ensure that the following steps are all taken care of before submitting
the PR.

• [ ] This has been discussed in issue #..., or if not, please tell us why
  here.

• [ ] A short description of what this does, why it is needed; if the
  description becomes long, the matter should probably be discussed in an issue
  first.

• [ ] This PR contains test cases, if meaningful.

• [ ] A reviewer from the core maintainer team has been assigned for this PR.
  If you don't know who could review this, please indicate so. The list of
  suggested reviewers on the right can help you.

Please ensure all communication adheres to the code of conduct.
-->

Wasmtime GitHub notifications bot (Jun 03 2020 at 22:36):

pchickey submitted PR Review.

Wasmtime GitHub notifications bot (Jun 03 2020 at 22:36):

pchickey created PR Review Comment:

Thanks, didn't know that was the preferred syntax but it is a lot clearer. Fixed.

Wasmtime GitHub notifications bot (Jun 03 2020 at 22:36):

pchickey submitted PR Review.

Wasmtime GitHub notifications bot (Jun 03 2020 at 22:36):

pchickey created PR Review Comment:

Fixed Thanks

Wasmtime GitHub notifications bot (Jun 03 2020 at 22:36):

pchickey created PR Review Comment:

Made the comment clearer. Thanks

Wasmtime GitHub notifications bot (Jun 03 2020 at 22:36):

pchickey submitted PR Review.

Wasmtime GitHub notifications bot (Jun 03 2020 at 23:04):

hawkw submitted PR Review.

Wasmtime GitHub notifications bot (Jun 03 2020 at 23:04):

hawkw created PR Review Comment:

The dependency tree could also be pruned a bit more by disabling the crate's default features. E.g. if we don't care about timestamps, disabling them would remove the dependency on chrono, and so on.

Wasmtime GitHub notifications bot (Jun 04 2020 at 16:09):

alexcrichton submitted PR Review.

Wasmtime GitHub notifications bot (Jun 04 2020 at 16:09):

alexcrichton created PR Review Comment:

Nah sorry I didn't realize that this was all pulled in exclusively through a dev-dependency, that's fine to do!

Wasmtime GitHub notifications bot (Jun 04 2020 at 16:09):

alexcrichton submitted PR Review.

Wasmtime GitHub notifications bot (Jun 04 2020 at 16:44):

pchickey merged PR #1796.

Last updated: Nov 22 2024 at 17:03 UTC