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

Add V1 to V2 conversion logic. #100

Merged
merged 1 commit into from
Nov 9, 2018
Merged

Add V1 to V2 conversion logic. #100

merged 1 commit into from
Nov 9, 2018

Conversation

drolando
Copy link
Contributor

This logic is very convoluted and the code is not pythonic, however we
tried to stay as close as possible to the openzipkin/zipkin
implementation so that it'd be easier to port over future fixes.

@adriancole This is logic that @msindwan wrote a while ago to convert v1 thrift spans to v2 json. This PR is still a work in progress, but can you have a look at whether the code makes sense.
We didn't really know what a bunch of these edge cases protect against, but decided it was best to just replicate the exact same code as zipkin-server.

Also, I'm adding this here due to the discussion we had last Thursday of providing tools to convert v1 to v2. But I'm also fine if we decide this shouldn't live here


def _write_hex_long(data, pos, value):
"""
Writes an unsigned long value across a byte array.

Choose a reason for hiding this comment

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

might want to qualify big endian

Copy link

@codefromthecrypt codefromthecrypt left a comment

Choose a reason for hiding this comment

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

PS I am happy once you are comfortable with unit test coverage

@drolando
Copy link
Contributor Author

I've rewritten most of this logic since the previous version had several errors and I couldn't understand it. The new code might be missing some corner cases, but we can add those as we find them. Some of the corner cases were for merged client/server which we should never encounter since the merging is now done in the ui.

I realized that since SpanBuilder and the various encoders already knew how to encode to any format, if I could decode the raw spans in a list of SpanBuilders I could easily reuse the old code.

@drolando
Copy link
Contributor Author

Test coverage is still < 100%, I'll try to fix that. I'm also not yet 100% sure this code works correctly every time. I'll run it a bit more in dev and make sure the output is reasonable.

@drolando
Copy link
Contributor Author

Also, the diff is huge since I moved _encoding_helpers into an encoding folder and github shows it as a file being deleted and a new one created. I'll try to create another PR for this move so that this diff is more manageable.

@codefromthecrypt
Copy link

codefromthecrypt commented Oct 18, 2018 via email

drolando added a commit to drolando/py_zipkin that referenced this pull request Oct 18, 2018
This will better split out the logic and make it easier to add decoders
in Yelp#100
@coveralls
Copy link

coveralls commented Nov 8, 2018

Coverage Status

Coverage remained the same at 100.0% when pulling 5635cb0 on add_v2_conversion_logic into 7da811c on master.

_V2_ATTRIBUTES = ["tags", "localEndpoint", "remoteEndpoint", "shared", "kind"]


def detect_span_version_and_encoding(message):

Choose a reason for hiding this comment

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

your code is neater than mine :D

@drolando drolando merged commit fce3c9f into master Nov 9, 2018
@drolando drolando deleted the add_v2_conversion_logic branch January 30, 2019 06:35
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.

None yet

3 participants