Change context missing strategy behaviour to Log Error #83
Conversation
test/aws-xray-sdk/tc_context.rb
Outdated
| @@ -20,14 +20,15 @@ def test_change_context_missing | |||
| context = XRay::DefaultContext.new | |||
| context.clear! | |||
| context.context_missing = 'UNKWON' | |||
There was a problem hiding this comment.
It is, but I cannot tell if it is deliberate. The purpose of this line is to set a random invalid context_missing value, so this typo could either be on purpose or not. Setting it to 'UNKNOWN' will yield the same result.
There was a problem hiding this comment.
Makes sense. If they both yield the same result, maybe it would be better to just correct the typo for readability? What do you think?
There was a problem hiding this comment.
Changed to 'INVALID_STRATEGY'
| @@ -36,7 +37,6 @@ def test_runtime_error | |||
| def test_log_error | |||
| context = XRay::DefaultContext.new | |||
| context.clear! | |||
| context.context_missing = 'LOG_ERROR' | |||
There was a problem hiding this comment.
Since this test is called test_log_error, perhaps adding an assertion that context.context_missing is LOG_ERROR by default would also make sense
There was a problem hiding this comment.
Good point. Originally test_runtime_error assumed the default value, so I thought to test test_log_error assuming the default value as well with the new change. I think it makes sense to incorporate your suggestion, and then add a test_default_context_missing test case.
| def test_default_context_missing | ||
| context = XRay::DefaultContext.new | ||
| context.clear! | ||
| refute context.current_entity |
There was a problem hiding this comment.
Is there an easy way to see if a log was added here?
There was a problem hiding this comment.
Good callout. The XRay Ruby SDKs don't test the output of the logger anywhere else unfortunately, it won't be a quick change. This can be tackled in another enhancement.
There was a problem hiding this comment.
Sounds good. Feel free to merge this then.
Issue #, if available:
Description of changes:
This change sets the default context missing strategy to 'LOG_ERROR' instead of 'RUNTIME_ERROR' to mitigate malformed trace headers from breaking services.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.