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
tracer: Check for nil interface types in span.Finish(WithError()) #2036
base: main
Are you sure you want to change the base?
Conversation
BenchmarksBenchmark execution time: 2024-01-10 19:56:21 Comparing candidate commit f5901b4 in PR branch Found 0 performance improvements and 3 performance regressions! Performance is the same for 36 metrics, 2 unstable metrics. scenario:BenchmarkTracerAddSpans-24
|
@ajgajg1134 did you ultimately decide to abandon this PR, or do you still want review in it's current state? I remember there being discussion, but don't recall our final decision on that. |
I believe we decided to move forward with our best effort approach here, but I had some unresolved (and unposted) comments in my review here from our discussion. I've just posted and addressed them so I would say this is ready for final review. |
In looking at this again I realized our existing benchmarks didn't actually really cover this
This seems like a reasonable change to me? Less than 1% and it's only incurred when the error is non-nil |
@ajgajg1134 do you want to add those benchmarks here, so we don't lose them? Agreed that 1% isn't a big enough concern to worry about here, so should be good to merge it. |
What does this PR do?
Fixes a bug where if a user created a nil type of their own implementation of the
error
interface the nil check we do is not sufficient to ensure we do not panic later. (For context checkout this old thread which includes a playground link)Motivation
Closes #2029
Describe how to test/QA your changes
Covered with Unit test
Reviewer's Checklist