pchickey commented on Issue #1796:
Currently a very sketchy WIP. I made an example inside test-helpers (because it was most convenient to reuse the test-helpers HostMemory and WasiCtx) that demonstrates that tracing integration works at all, but I'm not sure where the best place to stick that in the long run is. In order to provide an example we have to take on
tracing
andtracing_subscriber
as adev-dependency
, which are not especially heavyweight.
pchickey edited a comment on Issue #1796:
Currently a very sketchy WIP, just wanted to show I could factor it out to provide either logging setup as a config option to the macro. Need to still finish factoring out all the log statements into helper funcs, and also setup tracing spans per function.
I made an example inside test-helpers (because it was most convenient to reuse the test-helpers HostMemory and WasiCtx) that demonstrates that tracing integration works at all, but I'm not sure where the best place to stick that in the long run is. In order to provide an example we have to take on
tracing
andtracing_subscriber
as adev-dependency
, which are not especially heavyweight.
github-actions[bot] commented on Issue #1796:
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>
kubkon commented on Issue #1796:
This looks interesting, thanks Pat! I haven’t looked over the code properly yet, however, one observation about removing
log
altogether is that this will probably force every user oflibwasmtime
to rely ontracing
crate which i don’t think is a good idea. I think if I was a developer making use of the library, I’d rather have the choice or at the very least expectlog
support by default which seems to be the “industry” standard.
alexcrichton commented on Issue #1796:
This seems reasonable to me to add as a feature, but I'm also hesitant about deleting
log
. It looks liketracing
is a pretty hefty dependency, and I'm not sure all users ofwasmtime
will want to take that on.(stable) support for structured fields
FWIW this seems like a good reason to poke upstream about the support here, I know it's been in limbo for awhile but upstream may have thoughts about stabilizing.
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.
I know this is also true for
log
, so I don't think there should be any speed difference.
pchickey commented on Issue #1796:
Cool, thanks for the input and the context. I'll move forward with this patch which will make the logging choice an either/or
log
/tracing
. I'm not going to pull on the thread of structured fields inlog
for lack of time to do so, but I'd welcome any help with that.
pchickey commented on Issue #1796:
I thought about this more and got some feedback from @hawkw and @fitzgen -
- taking a dep on
tracing
impliestracing-core
andtracing-attributes
, which in turn pull inlazy_static
,cfg-if
, and the standard proc macro crates (proc-macro2, syn, quote).wasi-common
already has all of those deps transitively, so, this is not so heavyweight.tracing-subscriber
is the heavyweight crate, just like the backend crates in thelog
ecosystem.- when the
log
feature is enabled fortracing
, thelog
system is used as the backend for thetracing
macros. This allows the existing users who just wantlog
in their system to use wasi-common just as they did before. Since thetracing
dep isn't very heavy, this seems preferable than having a switch throughout wiggle codegen to emit one kind of log or another- If we want to completely disable any code footprint from
tracing
orlog
, Wiggle can emit macros that point towiggle::tracing_impl::(macro)
, and use a feature flag inwiggle/src/tracing_impl.rs
to substitute betweenpub use tracing::*
and a module that defines no-op versions of each macro. This is suitable for use cases that are very sensitive about vendoring in additional deps, or where the code size of always-runtime-disabled macros is an issue.
pchickey commented on Issue #1796:
Unfortunately, we can't switch between the
tracing
macros and no-op versions, while keeping that api re-exported inside a module scope likewiggle::tracing
(and its a fairly big API, so i think we really need to), without defining a whole othertracing_noop
crate, because much of the API is in terms ofmacro_rules
which can only be exported at the top level of a crate. So, Rust visibility rules got us here.At some point, the cranelift project will need to implement a
tracing_noop
crate in order to turn offtracing
deps for use inside spidermonkey. Until then, we'll do a mandatory dep ontracing
, with just a feature to set it inlog
mode.
pchickey commented on Issue #1796:
The failures in
wasi-common
are due to a fixed bug intracing
that will be part of the 0.1.15 release, which will happen shortly since we need it to merge this.
hawkw commented on Issue #1796:
The failures in
wasi-common
are due to a fixed bug intracing
that will be part of the 0.1.15 release, which will happen shortly since we need it to merge this.Hot off the presses: https://crates.io/crates/tracing/0.1.15
Sorry for any inconvenience! :)
pchickey commented on Issue #1796:
I believe this is ready for review.
The example program at
crates/wiggle/test-helpers/examples/tracing.rs
demonstrates the use oftracing-subscriber
andlog
as backends to the tracing statements emitted by wiggle. If you set theRUST_LOG
environment variable, itll useenv_logger
to output, otherwise it'll usetracing-subscriber
.
Last updated: Dec 23 2024 at 13:07 UTC