-
Notifications
You must be signed in to change notification settings - Fork 375
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Datadog.tracer.active_correlation not available in Sidekiq logging #1068
Comments
FYI I don't think the stale tracer is the cause here. In the Sidekiq source, https://github.com/mperham/sidekiq/blob/master/lib/sidekiq/processor.rb#L161 |
Yeah, after looking at this I arrived at the same conclusion, I think we'll have to investigate this separately. |
@jeffblake we found this recently as well, I was wondering if you implemented the thread variable successfully? |
👋 @jeffblake and @wasabigeek, as @jeffblake pointed out,
Given that, your This seems to me like there's a two tiered systems of Sidekiq server-side hooks:
One approach is for us to move our Datadog middleware to outside the context of Sidekiq middleware: we could make Whenever a library has proper middleware, like Sidekiq does, using them is normally a much better maintainability experience for both users and library maintainers. The main issue I see in this case is that a custom I took a look at the interface provided by both Sidekiq::JobLogger and the middleware interface and, given all the information is available to the middleware as far as I can tell, my suggestion here would be to move the logic from the Let me know if I'm missing something in your use case that can't be covered by this suggestion. |
@marcotc Your analysis seems spot on. The sidekiq creator acknowledged this when I brought it up, but expressed no interest to go back to logging being a middleware (which is the better design in my opinion).
Regarding your last paragraph, correct me if I'm wrong, but doing this in middleware seems like a no-go, because all middleware thread-variables would be flushed before the JobLogger gets a chance to log the span/trace ID? Or are you saying move logging out of JobLogger altogether? I don't like that because I prefer to follow whatever design Sidekiq gives us, whether it's correct or not... |
Is this fixed? |
@icelynjennings if you use Sidekiq through Active Job, then it is sort of fixed. Most logs are correlated through Active Job integration. If, however, you're using Sidekiq "natively", then unfortunately it isn't fixed. |
Thank you for your ActiveJob solution @agrobbin. I wonder if the recent solving of this Sidekiq issue sidekiq/sidekiq#5021 via commit sidekiq/sidekiq@90535ab could somehow be leveraged to inherit Rails DD tracing into sidekiq logs |
@icelynjennings it'd be great if the functionality could be expanded, but I don't think the solution in sidekiq/sidekiq@90535ab actually does anything to make that easier. It broadcasts any |
For some more context: what information are we missing today because of the lack of trace-logs correlation at the job logger level? Is it only these three ("start", "done", "fail") log lines: https://github.com/mperham/sidekiq/blob/1f05bd60102419d12733afa267d96524ece8e756/lib/sidekiq/job_logger.rb#L11-L20. Or is there more rich data that is going uncaptured today? |
Hi @marcotc, I'm new to sidekiq so I cannot answer but a DataDog support ticket I had filed led me to believe that native sidekiq log correlation is not currently available in DD. What I'm missing at the moment is any traceID within our sidekiq job logs. |
Just my personal experience: I've got a lot of work done within workers, multiple queues, multiple workers per queue, multiple threads per worker, but the only context I get in DD is that it's Worker.x. We really need to be able to isolate what each individual thread in each worker in each queue is doing over time. "Filter logs by thread x on worker y in queue z" is the real function I need. |
Hey all, does anyone know if anything has changed about this in the past year and a half and if it's possible to now correlate sidekiq logs? |
With Sidekiq 5 (released in 2017), job logging is no longer a middleware, and now sits above all middleware in the stack.
I think this is why
Datadog.tracer.active_correlation
is not available in the new JobLogger, because the correlation is flushed in theSidekiq::ServerTracer
middleware.Seems crazy that no one has noticed this before, so maybe I have it wrong?
My setup is like
Datadog.tracer.active_correlation
is always 0 for space_id and trace_id in theJobLogger
Datadog.tracer.active_correlation
in myLogging::Sidekiq::Server::JobLogger
middleware has a span_Id, but it seems to be one of the inner spans, e.g. a postgres query.I suppose I could grab the root span in my custom middleware, store it in
Thread.current[:dd_trace] = Datadog.tracer.active_root_span
and then retrieve it in theJobLogger
class for logging, (with anensure
block to flush it) ? But since other people must have this problem and I can't find a documented solution, thought I'd bring it up here.The text was updated successfully, but these errors were encountered: