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

Misc changes #5

Merged
merged 4 commits into from
Aug 10, 2018
Merged

Misc changes #5

merged 4 commits into from
Aug 10, 2018

Conversation

james-callahan
Copy link
Collaborator

@james-callahan james-callahan commented Aug 9, 2018

  • Remove trailing whitespace
  • Rename test/ to spec/ to follow busted convention
  • Have :log_kv take an optional timestamp
  • Add span:each_baggage_item()

@rnburn
Copy link
Collaborator

rnburn commented Aug 9, 2018

Most of those changes look good, but the other OT APIs don't have functions to lookup the tags and logs ... is there any reason we'd need them for Lua?

@james-callahan
Copy link
Collaborator Author

OT APIs don't have functions to lookup the tags and logs

It seems other languages have flirted with a get_tag:

See opentracing/specification#12 for :each_baggage_item

@rnburn
Copy link
Collaborator

rnburn commented Aug 9, 2018

I see the discussion for tag accessors, but I checked the Go and Java APIs and don't see that any methods were ever added to the interface for them.

I'm fine with a function to iterate over the baggage -- that's pretty standard.

@james-callahan
Copy link
Collaborator Author

Rebased to have commits you're not sure about at the end (that way you can merge just the first commits if you want)

@rnburn
Copy link
Collaborator

rnburn commented Aug 9, 2018

I'm not sure how to do a partial merge like that. Could you just remove the get_tag, each_tag, and each_log functions? The other changes look good.

@james-callahan
Copy link
Collaborator Author

Done. once merged I'll send another PR for those bits.

@rnburn rnburn merged commit ae354ad into opentracing:master Aug 10, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants