Skip to content
This repository was archived by the owner on Jan 10, 2025. It is now read-only.

Consider dropping the release_max_level_warn feature from tracing dependency #242

Closed
flavio opened this issue Mar 15, 2023 · 8 comments
Closed

Comments

@flavio
Copy link

flavio commented Mar 15, 2023

Hello, I'm one of the maintainers of kubewarden, I'm reaching out because we are using mdcat as a library inside of our kwctl command line utility.

mdcat is doing an amazing job at rendering markdown, this is the best solution we found out there. Congratulations! ❤️
This is why we are doing something kinda dirty, we're consuming the mdcat crate to implement the kwctl inspect command (source code here).

I recently discovered that all our tracing::info output was now shown when kwctl was compiled in release mode.
After some spelunking I found the mdcat dependency is causing that because of this line:

tracing = { version = "0.1.30", features = ["release_max_level_warn"] }

I was wondering if you could drop the release_max_level_warn feature from the tracing dependency. I've seen that inside of your main.rs you're actually disabling any kind of trace event:

mdcat/src/bin/mdcat/main.rs

Lines 164 to 170 in 28a2d25

// Setup tracing
let filter = EnvFilter::builder()
// Disable all logging by default, to avoid interfering with regular output at all cost.
// tracing is a debugging tool here so we expect it to be enabled explicitly.
.with_default_directive(LevelFilter::OFF.into())
.with_env_var("MDCAT_LOG")
.from_env_lossy();

Shouldn't that be enough? I tried patching the Cargo.toml and from some quick tests everything seemed fine (as in, no trace event was printed).

Another possible solution, a better one IMHO, would be to move mdcat libraries to a dedicated crate that could then be consumed by third parties like us. What do you think about that? 🥺

Again, thanks a lot for the awesome job you did with this project!

@flavio flavio changed the title Consider chaging dropping the release_max_level_warn feature from tracing dependency Consider dropping the release_max_level_warn feature from tracing dependency Mar 15, 2023
@swsnr
Copy link
Owner

swsnr commented Mar 16, 2023

Thank you for your kind words 🙂

I'm not sure I would not like to disable that feature; mdcat does a lot of internal tracing in the rendering code to help me understand rendering states, and I'm not sure if it's a good idea to have all that in a release build 🤷🏿

But I'd happily consider a pull request which splits out the library code, i.e. lib.rs and downwards, into a separate say pulldown-cmark-tty crate or so, and sets up all the cargo workspace stuff, also for Github actions and cargo release. I might do this myself at some point, but it will take a while, since mdcat is a very side project for me.


Also, I have only a coarse idea what kubewarden is, but your website looks very professional and talks about "policy" and "admission", so I presume it's some enterprise-level security-related thing, and I feel like I should have things straight: mdcat comes with absolutely no warranty. Any bugs or security issues you find in mdcat in your project are yours to fix, and any kind of DCO, SBOM or whatever bureaucracy that sometimes goes along with enterprise-level open source things is yours to do, and you will likely have to wait long time for me to review any pull requests. Do not expect me to do anything here.

If you expect me to do even just basic maintenance here, I'd prefer if you didn't use mdcat 🙂

I'm sorry for being blunt, more so, if my guestimate about kubewarden is entirely wrong 😇 but I think it's important to clarify what you can expect from me (nothing really 😇 ) if you're using mdcat in a project at enterprise level.

@flavio
Copy link
Author

flavio commented Mar 16, 2023

I'm not sure I would not like to disable that feature; mdcat does a lot of internal tracing in the rendering code to help me understand rendering states, and I'm not sure if it's a good idea to have all that in a release build 🤷🏿

In theory, filtering the trace/debub/info statement using a log level doesn't harm the performance of the final binary. The compilation feature you're using is kinda of a last resort. All of that according to the information provided ~6 months ago on this reddit comment by one of the maintainers of tracing.

I don't think mdcat needs to squeeze all the juice out of the CPU, I think it would perform fine also without using this flag.

Just to be clear, I don't want to push you into doing that. I'm just thinking out loud.

But I'd happily consider a pull request which splits out the library code, i.e. lib.rs and downwards, into a separate say pulldown-cmark-tty crate or so, and sets up all the cargo workspace stuff, also for Github actions and cargo release. I might do this myself at some point, but it will take a while, since mdcat is a very side project for me.

I totally get it. I can set aside some time to provide a PR that does these changes. Given this is a big change, I wanted to be sure you would be fine with this idea before starting to implement that. It's never good to show up with a big amount of invasive changes without announcing yourself :)

I'm sorry for being blunt, more so, if my guestimate about kubewarden is entirely wrong innocent but I think it's important to clarify what you can expect from me (nothing really innocent ) if you're using mdcat in a project at enterprise level.

Again, I totally get it. I do have some spare time open source projects too, I feel your pain and I understand your concerns.

However, in our case the usage of mdcat is well defined and definitely not security critical. Just to give you the full picture, we use mdcat to render the Markdown README that is embedded into our policies.

swsnr added a commit that referenced this issue Mar 16, 2023

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
Closes GH-242
@swsnr swsnr closed this as completed in 39d43a3 Mar 16, 2023
@swsnr
Copy link
Owner

swsnr commented Mar 16, 2023

Thanks for providing some background on the effect of this feature. I think you're right; mdcat doesn't need it.

I've pushed a commit which removes the feature, and another one which uses default-features = false for all dependencies, just to avoid another feature getting in your way.

I'll make a new release over the next days.

Again, I totally get it. I do have some spare time open source projects too, I feel your pain and I understand your concerns.

Thanks for understanding ❤️

@flavio
Copy link
Author

flavio commented Mar 17, 2023

I've pushed a commit which removes the feature, and another one which uses default-features = false for all dependencies, just to avoid another feature getting in your way.

I'll make a new release over the next days.

Wow, thanks a lot!

@swsnr
Copy link
Owner

swsnr commented Mar 18, 2023

1.1.1 is out.

@swsnr
Copy link
Owner

swsnr commented Apr 15, 2023

@flavio Just wanted to make you aware that I've split out the rendering code into a new pulldown-cmark-mdcat crate, and moved out a bunch of heavy dependencies (reqwest) and guarded other heavy dependencies behind features (resvg, image).

You'll need to replace the mdcat with pulldown-cmark-mdcat, and building with default-features = false, features = ["regex-fancy"] should give you a considerably lighter build (at the cost of reduced image support, no SVG support, and no HTTP support for images).

I'll release this a 2.0.0 soonish.

@flavio
Copy link
Author

flavio commented Apr 17, 2023

Thanks! I'll wait for mdcat 2.0.0 to be out, so that I can learn how to use the new crate by looking at its source code

@swsnr
Copy link
Owner

swsnr commented Apr 17, 2023

It's out already.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants