Add utility executors that can be used with asynchronous frameworks t… #150
Conversation
…o propagate segment context.
There was a problem hiding this comment.
Really great feature idea! We'll have to update these docs for the next release to include this as an option.
|
|
||
| /** | ||
| * {@link Executor}s that will mount a segment before running a command. When switching threads, for example when instrumenting an | ||
| * asynchronous appilcation, it is recommended to use one of these {@link Executor}s to make sure callbacks have the trace segment |
There was a problem hiding this comment.
Nit: typo in application
There was a problem hiding this comment.
Haven't used Mac in a while and now I'm back to hating this keyboard...
| } | ||
|
|
||
| private static ValidationMode validationMode() { | ||
| String config = System.getProperty("com.amazonaws.xray.validationMode", "").toLowerCase(); |
There was a problem hiding this comment.
Just curious, why not use ValidationMode.valueOf(config) instead of the switch statement?
There was a problem hiding this comment.
No real reason, switched to using it. After switching, realized the straightforwardness was lost and wrote unit tests instead so not sure which is better - oh well :)
| private static final ValidationMode VALIDATION_MODE = validationMode(); | ||
|
|
||
| /** | ||
| * Returhs whether {@code obj} is {@code null}. |
| recorder.setTraceEntity(segment); | ||
| try { | ||
| command.run(); | ||
| } finally { |
There was a problem hiding this comment.
It would be cool if the executor caught any throwables and added them to the segment with segment.addException(throwable) before rethrowing.
There was a problem hiding this comment.
I'm wary of this since it could result in double logging I think. For example, if it was a spring app I'd expect a spring exception handler to be listening to unhandled exceptions and adding to the segment appropriately. That being said, maybe we don't have any instrumentation that does this. Let me know if you still think this is a good place for such handling.
There was a problem hiding this comment.
That's a fair point. I was thinking about how we add the exception in function capturing (sub)segments here, but we also create the (sub)segments in those cases rather than using the current one in the context. Since using the segment in the context could risk double adding exceptions, e.g. for Spring segments like you mentioned, I think it's ok as is.
…o propagate segment context.
I noticed that we don't have any convenience executors for context propagation across threads. While they are not the only way, context propagating executors tend to be a good low level primitive for context propagation (works decently with CompletableFuture, RxJava, etc).
I may move some methods around later but want to get this in as a start.
I also added a loose validation utility to allow us to validate arguments with optional errors.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.