Skip to content

fix(profiling): check long string before allocating#3561

Merged
morrisonlevi merged 3 commits intomasterfrom
levi/extract_function_name
Jan 12, 2026
Merged

fix(profiling): check long string before allocating#3561
morrisonlevi merged 3 commits intomasterfrom
levi/extract_function_name

Conversation

@morrisonlevi
Copy link
Copy Markdown
Collaborator

@morrisonlevi morrisonlevi commented Jan 9, 2026

Description

It's rare but we sometimes see extract_function_name in crash reports due to failed memory allocations. At the moment, refactoring this function return a Result<T, E> instead of T would be difficult because of its callers like common_labels which can't fail either.

But I did notice an easy fix for some cases: move the large string check earlier. This won't fix OOMs due to genuinely hitting the memory limits, but it will reduce the damage caused by bugs letting through large strings in the first place, which are usually errors.

I also renamed [large string] to [suspiciously large string] to indicate that these are typically bugs. Class + method names are typically not 64 KiB or larger in size, nor are file names.

Reviewer checklist

  • Test coverage seems ok.
  • Appropriate labels assigned.

@morrisonlevi morrisonlevi added profiling Relates to the Continuous Profiler crashtracker-identified labels Jan 9, 2026
@morrisonlevi morrisonlevi requested a review from a team as a code owner January 9, 2026 21:07
@datadog-datadog-prod-us1
Copy link
Copy Markdown

datadog-datadog-prod-us1 Bot commented Jan 9, 2026 •
edited by datadog-official Bot
Loading

⚠️ Tests

✨ Fix all issues with Cursor

⚠️ Warnings

🧪 2141 Tests failed

    testSearchPhpBinaries from integration.DDTrace\Tests\Integration\PHPInstallerTest (Fix with Cursor)

    testSimplePushAndProcess from laravel-58-test.DDTrace\Tests\Integrations\Laravel\V5_8\QueueTest (Fix with Cursor)

testSimplePushAndProcess from laravel-8x-test.DDTrace\Tests\Integrations\Laravel\V8_x\QueueTest (Datadog) (Fix with Cursor) 
DDTrace\Tests\Integrations\Laravel\V8_x\QueueTest::testSimplePushAndProcess
Test code or tested code printed unexpected output: spanLinksTraceId: 6964b89300000000870100a5c909942d
tid: 6964b89300000000
hexProcessTraceId: 870100a5c909942d
hexProcessSpanId: 86f7069ac17498f2
processTraceId: 9728057382139434029
processSpanId: 9725249182070249714

phpvfscomposer://tests/vendor/phpunit/phpunit/phpunit:106
View all

ℹ️ Info

❄️ No new flaky tests detected

This comment will be updated automatically if new data arrives.
🔗 Commit SHA: fa1f099 | Docs | Datadog PR Page | Was this helpful? Give us feedback!

@pr-commenter
Copy link
Copy Markdown

pr-commenter Bot commented Jan 9, 2026 •
edited
Loading

Benchmarks [ profiler ]

Benchmark execution time: 2026-01-12 09:03:32

Comparing candidate commit fa1f099 in PR branch levi/extract_function_name with baseline commit 3112dff in branch master.

Found 0 performance improvements and 0 performance regressions! Performance is the same for 27 metrics, 9 unstable metrics.

@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Jan 9, 2026 •
edited
Loading

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 61.81%. Comparing base (3112dff) to head (fa1f099).

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3561      +/-   ##
==========================================
- Coverage   61.91%   61.81%   -0.11%     
==========================================
  Files         140      140              
  Lines       13281    13281              
  Branches     1758     1758              
==========================================
- Hits         8223     8209      -14     
- Misses       4269     4282      +13     
- Partials      789      790       +1

see 3 files with indirect coverage changes

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3112dff...fa1f099. Read the comment docs.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

realFlowControl
realFlowControl approved these changes Jan 12, 2026
View reviewed changes
Copy link
Copy Markdown
Member

@realFlowControl realFlowControl left a comment

Choose a reason for hiding this comment

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

I've added some tests, looks good, thanks 🙇

@realFlowControl realFlowControl force-pushed the levi/extract_function_name branch from aa59da0 to fa1f099 Compare January 12, 2026 08:50
@morrisonlevi morrisonlevi merged commit 7ae3874 into master Jan 12, 2026
2005 of 2008 checks passed
@morrisonlevi morrisonlevi deleted the levi/extract_function_name branch January 12, 2026 16:38
@github-actions github-actions Bot added this to the 1.16.0 milestone Jan 12, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@realFlowControl realFlowControl realFlowControl approved these changes

Assignees

No one assigned

Labels

crashtracker-identified identified-by:crashtracking profiling Relates to the Continuous Profiler tracing

Projects

None yet

Milestone

1.16.0

Development

Successfully merging this pull request may close these issues.

3 participants

@morrisonlevi @codecov-commenter @realFlowControl