Skip to content
This repository was archived by the owner on Jul 11, 2022. It is now read-only.

fix(python3): add support. skindeep changes for #59 #99

Merged
merged 2 commits into from
Nov 24, 2017

Conversation

dudymas
Copy link
Contributor

@dudymas dudymas commented Nov 21, 2017

This has been lightly tested, and will hopefully foster discussion. I'm doing a spike for adding tracing to our team's django app with Jaeger and Python3 .

I'd like to merge this in, but please understand I'm only doing this to get around opentracing-contrib/python-django#7 . I have another PR (to another PR) for that issue:
opentracing-contrib/python-django#6
granduke/python-django#1 (this is the changes I've made to add python3 support and such)

@dudymas dudymas mentioned this pull request Nov 21, 2017
@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) to 94.264% when pulling fc615e8 on dudymas:master into 988086b on jaegertracing:master.

@yurishkuro
Copy link
Member

most of these changes are to auto-generated files. How did you do that, manually?

@dudymas
Copy link
Contributor Author

dudymas commented Nov 22, 2017

Yeah! ooph... I thought 0.9 wasn't working for python 3. I just checked, and 0.10.0 is available. one min while I test it out, then I'll update the branch. woah nelly... it changed a lot of the generated code!

@dudymas
Copy link
Contributor Author

dudymas commented Nov 22, 2017

Unfortunately, the 0.10.0 version of thrift seems to break functionality. And it also appears that it would require updates to the jaeger service...?
So that's a no go. The easiest solution, if you don't approve of the manual edits, would be to run a sed script after the thrift generation is done. Thoughts? That'd be easy to whip up...

@yurishkuro
Copy link
Member

iirc (based on #59) at runtime you can use Py3 with thrift-.10 while the classes are still generated by 0.9.3, but yes they do need to be fixed up a bit (see #59 (comment)). If fixer-upper can be done with sed or otherwise automated then I am not against doing that.

@dudymas
Copy link
Contributor Author

dudymas commented Nov 22, 2017

Okay, see what you think. I found that sed on osx will break with linux unless I add a backup extension... thus I just went ahead and added that. Should work fine on all major three: mac/linux/wsl

@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) to 94.264% when pulling 5e6e659 on dudymas:master into 988086b on jaegertracing:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) to 94.264% when pulling 5e6e659 on dudymas:master into 988086b on jaegertracing:master.

@yurishkuro
Copy link
Member

lgtm. Could you please rebase off master so that the build is green?

@dudymas
Copy link
Contributor Author

dudymas commented Nov 23, 2017

rebased \o/ woohoo.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) to 94.264% when pulling d7a84a6 on dudymas:master into 2f780af on jaegertracing:master.

@yurishkuro yurishkuro merged commit c4908a4 into jaegertracing:master Nov 24, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants