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
Misc small changes #607
Misc small changes #607
Conversation
tylerbenson
commented
Nov 29, 2018
•
edited
edited
- Add null check for envelope in consumer delegate. Likely not a problem, but added just to be safe.
- Suggested by @ctoestreich.
- Fix possible NPE in adapter and guard against errors in the instrumentation.
- Added a different kind of 404 test to spring boot that passes routing, but returns 404.
This one succeeds in applying routing, but returns a 404 response.
Previously the delegate would swallow the exception and not rethrow. I also added a test to attempt to verify, but the exception doesn’t seem to be observable in the test. (See #602)
@@ -12,7 +12,9 @@ | |||
|
|||
public TextMapExtractAdapter(final Map<String, Object> headers) { | |||
for (final Map.Entry<String, Object> entry : headers.entrySet()) { | |||
map.put(entry.getKey(), entry.getValue().toString()); | |||
if (entry != null && entry.getValue() != null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why not just
for (final Map.Entry<String, Object> entry : headers.entrySet()) {
map.put(entry.getKey(), entry.getValue());
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// TextMap works with <String,String>, but the type we're given is <String,Object>
toString()
was the easiest way to achieve that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps this isn't a TextMap then. Is there an analogous <String, Object> type you have already created?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TextMap
is an Opentracing thing we've kind of inherited. Extending to Object would require more effort than I think is worth for this case.
@tylerbenson I suppose my other comments are irrelevant if these headers aren't repropgated and only used for trace. |