Closed EngRajabi closed 3 years ago
@Falco20019
@EngRajabi Will try to have a look today or tomorrow. I saw it yesterday but didn’t have time.
I reviewed changes and there is no problem.
I wanted to avoid using a separate timer and use a cancellation token instead to stop the running work. This way, the queue will not run over (only when it should). Otherwise it will create lots of pending tasks over time too.
Haven’t had time last week to thing about completely, which is why I haven’t commented it yet.
My changes pursued two goals First, if the block value reaches a maximum, it is no longer possible to send the update command. My second goal was to add a command separate from sending information to the server. This means that we send information to the server in two modes. 1- Reaching the number
Add cancellation token for thread
@yurishkuro How is the go client handling this? I checked it, and it also seems to have a cyclic queue triggering the sending. Is there a timeout in the transports or is it only starting them in a cycle and let them run indefinitely?
@EngRajabi I'm consulting with Yuri to make sure our solution feels similar to the other clients. I see that the go client is also using a timer internally. I just wasn't able to quickly find out if or how pending transports are cancelled.
I assume the main problem is not the RemoteReporter
, as this is implemented in accordance to the go client and behaves as specified and expected. I only assume that there should be a timeout on the Sender
which cancels it's flushing.
So it seems that you have a misconception about how the RemoteReporter
should behave in the part of how the queue should be filled and flushed. But I'm with you on the part that the Sender
should timeout the flushing at some point and mark those span's as dropped instead. I will make the change once the setup of the timeout is clarified in #193 directly, as this PR tries to solve something else and changing this in place would lead to confusion.
I assume you mean the AppendCommand
with "update", right?
No, I mean this. FlushCommand
This flush command should be skipped if the queue is full, as the FlushLoop
will add another one whenever there is space for it on the next cycle, just as the comment explains:
https://github.com/jaegertracing/jaeger-client-csharp/blob/master/src/Jaeger.Core/Reporters/RemoteReporter.cs#L96-L98
And there will be more space, when the current FlushCommand
being executed is not blocking indefinitely anymore. I explained it in more details in #193
@Falco20019 not sure if I fully follow your question. The basic pattern in the clients is this:
Originally this was all designed for UDP, so a timeout was not an issue. With the addition of HTTP transports the implementations may vary. I'd say ideally the HTTP submission should be async from the background thread itself, as long as the batch buffer is properly freed for the next batch while the submission is ongoing.
Also, there is some variety in how the timeouts are handled, some clients use command pattern in the queue.
Which problem is this PR solving?
Short description of the changes