search

hrxi / tracing-loki

A tracing layer for shipping logs to Grafana Loki
Apache License 2.0
50 stars 20 forks source link

Layer shutdown support #18

Closed afpro closed 1 year ago

afpro commented 1 year ago

shutdown Layer, let BackgroundTask exit gracefully.

hrxi commented 1 year ago

First of all, thanks for implementing this. It's definitely a wanted feature.

This PR unfortunately adds a lock into the common path of writing to the log. I'm not sure if it can be done without though.

The "avoid fixed crate version" is unnecessary, a snap = "1.0.5" requirement is the same as a snap = "^1.0.5" in Cargo.toml.

afpro commented 1 year ago

oh, idk crate version '^' is unnecessary, got it. and the lock, we need find a way let BackgroundTaks quit, release Sender is the most straight forward way. i thought about the performance too, Layer is definitely high frequency used. in my case, my project add a EnvFilter to loki layer, so this is not problem for me, but not ok for everyone. perhaps we can let shutdown BackgroundTask by a Notify or Semaphore?

tiagolobocastro commented 1 year ago

Would closing the channel help achieve the same? Some channels (eg: async_channel) allows us to close the channel through an immutable reference to the sender, if this one doesn't make we can close the receiver side instead?

afpro commented 1 year ago

async_channel use concurrent_queue， concurrent_queue implement immutable operation with 'atomic', seems good to me. @hrxi what do you think?

afpro commented 1 year ago

btw, we can use async_channel::unbounded() for channel full problem.

tiagolobocastro commented 1 year ago

Depending on how the receive-side is implemented here, I probably wouldn't recommend unbounded as you could end up infinitely increasing memory usage..

afpro commented 1 year ago

ok, then i will update this PR with a tokio::sync::Notify based solution, just quit BackgroundTask after drain all available data in channel.

afpro commented 1 year ago

i've tested this on my own project. just manually tested, no unit test.

hrxi commented 1 year ago

Looks good to me. Could you add the usage of the shutdown handle to an example? I think it'd be ready to get merged then.

afpro commented 1 year ago

Looks good to me. Could you add the usage of the shutdown handle to an example? I think it'd be ready to get merged then.

i'll add an example today or tomorrow

hrxi commented 1 year ago

Thanks for your persistence. :)