-
Notifications
You must be signed in to change notification settings - Fork 454
perf: refactor span tag setting #14324
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
Conversation
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good low hanging fruit here, I'll be curious to see if microbenchmarks are showing any improvements and if there are any SLOs we can lower from this (maybe not since it probably more impactful at scale, but I am curious)
only suggestion, can you add # PERF:
comments here to let people know we are doing span._metrics
and etc as an optimization and to not do anything differently
Bootstrap import analysisComparison of import times between this PR and base. SummaryThe average import time from this PR is: 265 ± 2 ms. The average import time from base is: 268 ± 2 ms. The import time difference between this PR and base is: -3.0 ± 0.1 ms. Import time breakdownThe following import paths have shrunk:
|
Performance SLOsCandidate: perf-improvements-span-tags (577e5b0) 🔵 No Baseline Data (24 suites)🔵 coreapiscenario - 12/12 (2 unstable)🔵 No baseline data available for this suite
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice change! Another performance optimization was can make is remove try-except blocks from set_metric and set_tags. Using exception handling for control flow is pretty slow. We can get 10-20x improvements in the worst case if we use conditional statements.
Also we can remove most internal usages of Span.set_tag. Set_tags_str should be used in most cases. Honestly we can even look into deprecating a lot of the side effects of set tag (setting metrics, sampling, etc.) in v4.0. That could be a big perf win. We should have other apis for this behavior.
Small performance wins by:
set_tag
orset_tags
; useset_tag_str
insteadset_metric
and set value directly on_metrics
when possibleChecklist
Reviewer Checklist