Closed sconover closed 5 months ago
FYI, contrib is the appropriate place for components like this https://github.com/open-telemetry/opentelemetry-python-contrib. It can't be merged into SDK.
@srikanthccv Thanks for the initial feedback, and I completely agree that it seems a little strange/wrong that this isn't in contrib. However there are a couple of key places where, given the way the code is structured, the code at present, unless I'm missing something, technically must be in here. I will cite the areas I see as issues, and I'd appreciate guidance on what to do about them. I'd be happy to put together a minimal PR to this repo to "prep" the lib for accommodation of the larger portion (all the jaeger remote sampling impl) which I'd put in the contrib repo.
Closing in favor of splitting this into this core PR and this contrib PR per @srikanthccv 's advice
DRAFT - DO NOT MERGE
There are a variety of challenging TODO's (marked
TODO(sconover)
) and, no doubt, changes to be made based on feedback from project maintainers.The goal of this initial set of changes is to conduct a pretty "straight" port of the corresponding code in jaeger-client-python, to get discussion going on how opentelemetry-python support for jaeger remote sampling should ultimately work.
Please see the jaeger-client-python project and especially sampler.py and test_sampler.py for the source of much of this port.
New dependencies:
I expect there's a very good chance that opentelemetry python docs will need to be updated in advance of an ultimate merge, and would appreciate advice on what those areas might be.
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
jaeger-client-python
Does This PR Require a Contrib Repo Change?
Answer the following question based on these examples of changes that would require a Contrib Repo Change:
Checklist:
I'm leaving all of the above "unchecked". I've made some style changes in the direction of this project and python 3, however I'm hesitant to "compromise" the relatively straight port at this point, to make comparison/contrasting easier for initial reviewers.
Regarding (even) unit tests: while this code is unit tested, the tests are ported, and stylistically are quite different from tests in this project - for example they make extensive use of mocks, among other differences.
Sample flask server and explanation: