-
Notifications
You must be signed in to change notification settings - Fork 345
Conversation
It's not a common scenario to use a Tag *key* as the tag value.
* Implement Trace Identifiers.
* Deprecate finishSpanOnClose on activation. * Add ScopeManager.activeSpan() and Tracer.activateSpan(). * Clarify the API changes and deprecations. * Add a error reporting sample to opentracing-testbed.
* Implement a simple layer on top of ByteBuffer for BINARY format.
Reduces the need to call Tags.SOMETAG.getKey() and adds type safety for setTag if done from the span. Fixes #271
* Allow injecting into maps of type Map<String,Object> For the inject case, it doesn’t matter so much that the generic type is declared as String. * Modify generic type on map in test.
This is done so we don't end up using an abstract class in the public API.
* Add simple registerIfAbsent to global tracer
It seems the previous one expired or was discarded.
If we had separate formats for inject and extract this refactor would have been much cleaner. This allows us to better avoid partially implementing the TextMap interface and throwing an exception for the other method. Since Binary is a new addition, I applied a similar refactoring to that. Closes #315.
* Deprecate ScopeManager.active() and update related code. * Rename common_request_handler to concurrent_common_request_handler. * Add a testbed case for request handlers having no storage for Scope.
* Make Tracer extends Closable.
* Update the README with the latest changes.
This one does not really need to be synchronized. The field itself is volatile and that is enough.
* Add a deprecation note for AutoFinishScopeManager/AutoFinishScope. * Add a clarification on why we are deprecating this. * Mention the Issue that lead to deprecating auto finish.
Hey - created a PR prior to releasing 0.32.0 (including updating the CHANGELOG). Let me know ;) |
Is this release fully backwards compatible? |
@felixbarny It should be compatible for users, but implementations will require some changes. |
Hey @felixbarny yes, it's as @tylerbenson said As an exception: for users, they will be impacted if they use inject/extract with adapters themselves, i.e. tracer.inject(context, Format.Builtin.TEXT_MAP, new TextMapInjectAdapter(map)); to: tracer.inject(context, Format.Builtin.TEXT_MAP_INJECT, new TextMapInjectAdapter(map)); // Or...
tracer.inject(context, Format.Builtin.TEXT_MAP, new TextMapAdapter(map)); |
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.
Unfortunately I don't have time to go over all changes in a single go, but assuming all these are PR's that were individually reviewed already, I approve
CHANGELOG.md
Outdated
* Split Inject and Extract builtin interfaces. | ||
* `Tracer` implements `Closable`. | ||
* Added `GlobalTracer.registerIfAbsent()`. | ||
* Added `GlobalTracer.isRegistered()`. |
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.
GlobalTracer.isRegistered()
already existed. The implementation changed.
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.
Oh, right. Thanks for the review. Will update the Changelog ;)
@@ -20,7 +20,7 @@ | |||
<parent> | |||
<groupId>io.opentracing</groupId> | |||
<artifactId>parent</artifactId> | |||
<version>0.31.1-SNAPSHOT</version> | |||
<version>0.32.0-RC3-SNAPSHOT</version> |
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.
Should we do an RC3?
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.
I think we are ready for an actual release - the latest (important) change from RC2 is making Tracer
implement Closable
;)
this should be added to the change log. can you do that? Also, It could be that |
@adriancole yes, both |
No description provided.