Skip to content
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 method and add log_kv method #8

Closed
wants to merge 2 commits into from

Conversation

Mahito
Copy link
Contributor

@Mahito Mahito commented Apr 2, 2018

opentracing-ruby deprecated log and added log_kv.
This PR add deprecated annotation and log_kv.

Mahito added 2 commits April 3, 2018 13:10
opentracing-ruby deprecated `log` and added `log_kv`.
This PR add deprecated annotation and `log_kv`.
@willmore willmore requested a review from indrekj April 3, 2018 10:49
# @param timestamp [Time] time of the log
# @param fields [Hash] Additional information to log
def log_kv(timestamp: Time.now, **fields)
@logs.push({
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how would this be used?

Copy link
Member

@indrekj indrekj Apr 3, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://github.com/openzipkin/zipkin-go-opentracing/blob/4c9fbcbd6d73a644fd17214fe475296780c68fb5/zipkin-recorder.go#L179-L185 This is what I found for go implementation but I'm not entirely sure what all of this does. Do you have any other examples?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@indrekj Thank you your comment.
I'm trying to use this library for our service tracing with event logging, however it doesn't implement .
I found an implementation in zipkin-go-opentracing too.
It seems to send logs that include timestamp and fields converted to strings in Annotations.
I could not find any other implementation, but since it is the official implementation of openzipkin I tried a similar implementation.

@indrekj
Copy link
Member

indrekj commented Apr 3, 2018

First commit merged with slight modifications ca0e76d . Thanks.

I currently will not merge the second commit as it doesn't define how it will actually be used and seems partial.

@indrekj indrekj closed this Apr 3, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants