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 overlog
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.
-->
alexcrichton submitted PR Review.
alexcrichton submitted PR Review.
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
)
alexcrichton created PR Review Comment:
I think this could perhaps be
pub extern crate tracing;
?
hawkw created PR Review Comment:
tracing-subscriber
is not required for the macros. It contains actualtracing::Subscriber
implementations (like loggers in thelog
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?
hawkw submitted PR Review.
hawkw edited PR Review Comment.
sunfishcode submitted PR Review.
sunfishcode submitted PR Review.
sunfishcode created PR Review Comment:
Please add spaces before and after the '='
hawkw edited PR Review Comment.
hawkw submitted PR Review.
hawkw submitted PR Review.
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_ toTRACE
. In order to enableDEBUG
and lower, you would usetracing::Level::DEBUG
here instead.
pchickey submitted PR Review.
pchickey created PR Review Comment:
Yes,
tracing-subscriber
gets pulled in because of thewiggle-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 droptracing-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 thelog
integration.
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 overlog
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.
-->
pchickey submitted PR Review.
pchickey created PR Review Comment:
Thanks, didn't know that was the preferred syntax but it is a lot clearer. Fixed.
pchickey submitted PR Review.
pchickey created PR Review Comment:
Fixed Thanks
pchickey created PR Review Comment:
Made the comment clearer. Thanks
pchickey submitted PR Review.
hawkw submitted PR Review.
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.
alexcrichton submitted PR Review.
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!
alexcrichton submitted PR Review.
pchickey merged PR #1796.
Last updated: Dec 23 2024 at 12:05 UTC