-
Notifications
You must be signed in to change notification settings - Fork 17
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
Deprecate log; add log_kv #23
Conversation
lib/opentracing/span.rb
Outdated
@@ -40,11 +40,20 @@ def get_baggage_item(key) | |||
nil | |||
end | |||
|
|||
# ** Deprecated ** |
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 be @deprecated
with a reason. See http://www.rubydoc.info/gems/yard/file/docs/Tags.md#deprecated
@@ -40,11 +40,20 @@ def get_baggage_item(key) | |||
nil | |||
end | |||
|
|||
# ** Deprecated ** | |||
# Add a log entry to this span | |||
# @param event [String] event name for the log | |||
# @param timestamp [Time] time of the log | |||
# @param fields [Hash] Additional information to log | |||
def log(event: nil, timestamp: Time.now, **fields) |
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 don't we remove event: nil
and not create a new log_kv method? Are we planning to use log
for something else? I think this should also be explained in the commit message.
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.
@indrekj Thanks for the feedback. Added reasons for the method name replacement in the code and commit message.
1. event is an optional log fields defined the the spec, not required 2. be consistent with other OT implementations such as Python and Go
In 0.4.0, opentracing-ruby deprecated Span#log in favour if Span#log_kv (opentracing/opentracing-ruby#23). (Closes #72)
In 0.4.0, opentracing-ruby deprecated Span#log in favour if Span#log_kv (opentracing/opentracing-ruby#23). (Closes #72)
PR for issue #22
@itsderek23 @mwear I moved the
assert_warn
method totest_helper
for sharing. There is a structured_warnings gem with more support warning methods. I don't think we need the gem for now but I can include it in the PR if that makes sense.