Skip to content

Extract null span context#81

Merged
jukylin merged 8 commits into
jukylin:masterfrom
cawolf:extract-null-span-context
Feb 1, 2020
Merged

Extract null span context#81
jukylin merged 8 commits into
jukylin:masterfrom
cawolf:extract-null-span-context

Conversation

@cawolf
Copy link
Copy Markdown
Contributor

@cawolf cawolf commented Dec 31, 2019

According to the opentracing specification, null may be returned instead of a SpanContext by Tracer->extract(). This PR adds this behaviour and prevents ugly warnings in Jaeger, which complains about invalid span references:

image

@jukylin
Copy link
Copy Markdown
Owner

jukylin commented Jan 5, 2020

How did you got this warnings, show your step and code.
You should add test for null result

@cawolf
Copy link
Copy Markdown
Contributor Author

cawolf commented Jan 6, 2020

Hm, to be honest, I cannot quite reproduce it with my setup anymore. Maybe it was a caching condition on my side.
Anyways, I added the suggested test cases.

@cawolf
Copy link
Copy Markdown
Contributor Author

cawolf commented Jan 30, 2020

Any news on this? Should anything else be changed?

@jukylin
Copy link
Copy Markdown
Owner

jukylin commented Jan 31, 2020

I should know how to reproduce this. if not, I don't know how to handle it.

@cawolf
Copy link
Copy Markdown
Contributor Author

cawolf commented Jan 31, 2020

Ok, it seems that another bug in my test application actually created that warning in jaeger, sorry for that misunderstanding!

So this PR is not about a bugfix, but about the exact implementation of the opentracing specification. Having this in mind, do you have any ideas on improving this PR?

@jukylin
Copy link
Copy Markdown
Owner

jukylin commented Jan 31, 2020

I use other way to detect when to return null, you can review the change with your branch.

@cawolf
Copy link
Copy Markdown
Contributor Author

cawolf commented Jan 31, 2020

I like that solution more than mine, that is fine for me 👍

@jukylin jukylin merged commit 39b3e83 into jukylin:master Feb 1, 2020
@cawolf cawolf deleted the extract-null-span-context branch February 1, 2020 12:21
@jukylin jukylin mentioned this pull request Mar 20, 2020
Closed
@jukylin jukylin mentioned this pull request Mar 31, 2020
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@cawolf @jukylin @jky-yy