add simple span processor #304
Conversation
Codecov Report
@@ Coverage Diff @@
## main #304 +/- ##
==========================================
+ Coverage 37.36% 37.95% +0.59%
==========================================
Files 41 49 +8
Lines 3198 3322 +124
==========================================
+ Hits 1195 1261 +66
- Misses 2003 2061 +58
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
|
Opened as a draft since it still needs some docs and I don't like that both processors are started in the supervisor no matter what (but that may be acceptable for now). |
tsloughter
force-pushed
the
simple-processor
branch
2 times, most recently
from
1a923e9 to
6d52e7b
Compare
The Simple Span Processor does a blocking call when ending a span that returns after the span is exported.
tsloughter
force-pushed
the
simple-processor
branch
from
6d52e7b to
9193a0f
Compare
| {keep_state, Data#data{runner_pid=RunnerPid, | ||
| current_from=From, | ||
| handed_off_table=OldTableName}, | ||
| [{state_timeout, ExportingTimeout, exporting_timeout}]}; |
There was a problem hiding this comment.
This implies that the events have individual timeouts for their send time, which ignores the queuing time. Is this accurate? I'm unfamiliar with the spec here.
But let's assume this is correct, then the desired behaviour, if I have process A and processes B sending a synchronous span at once, and both hit a connection timeout of 2 seconds, would mean that A would wait ~2 seconds, and B would wait ~4 seconds before returning.
There was a problem hiding this comment.
It is unrelated to the queue time, correct. This is a configuration on the processor to kill the exporter if it takes too long. I punted on a client side timeout that can be passed for now -- I think its possible to do it decently now with send_request or whatever it is that ensures old replies aren't a problem.
| {next_state, idle, Data#data{current_from=undefined, | ||
| runner_pid=undefined, | ||
| handed_off_table=undefined}, | ||
| [{reply, From, ok}]}. |
There was a problem hiding this comment.
I guess here we can already have sent the timeout result. The gen_statem module however supports a way to use a proxy process to hide the dirty messages:
Doing so would recommend using the option {clean_timeout, infinity} in gen_statem:call/3, which would set up a proxy process to ignore the duplicate messages. You might want to either use that, or make a special non-replying complete_exporting/1 function acknowledge timeout races and prevents sending two replies to a single call.
There was a problem hiding this comment.
Oh, thanks. I might use that, but this made me realize, don't I just need to set current_from to undefined in the timeout reply (which it currently isn't doing and is a bug)?
Then complete_exporting can not send a reply if the current_from is undefined.
There was a problem hiding this comment.
Actually, not right now, unless you have a way to either cancel the state timeout, or to change the state. As this is implemented right now, we set the timer, complete, stay in the same state.
So we're likely to hit an invalid leftover state timeout if there is a decent time between requests, such that sending one span, succeeding, responding okay, then waiting N seconds will still trigger a timeout message. The timeout is never cancel, the state never changes, so the state timeout should fire at some point. I'm not 100% what's the behaviour if you set a state timeout, then set a new one, whether that cancels the old state timeout or just adds more.
There was a problem hiding this comment.
Wait no, it repeats the state and is supposed to retry, so it can't set current_from to undefined... grr
There was a problem hiding this comment.
Yup, realized that after replying.
Setting the state timeout does cancel and create a new one though.
There was a problem hiding this comment.
Bleh, also, why am I sending an error if it repeats. Need to either not repeat or not return an error.
There was a problem hiding this comment.
Updated to transition to idle.
repeat was used in batch processor to do another export of newly ended spans that have been waiting while the current exporting was blocking them from being exported.
I realize now that might be the wrong way to go about since there should be an export timeout message in the queue since we postpone it if in the exporting state, so there should be no need to repeat the state. But I'll want to verify my thinking again on that and send a separate PR.
Since the simple_processor is only exporting a single span and does so based on a blocking call when a span ends there is no need to do a `repeat_state` after the exporting time out fails. The next export will be done if there is a span end message in the processor's queue.
The Simple Span Processor does a blocking call when ending a span
that returns after the span is exported.