Skip to content

Regular expressions can now be used for InApp includes/excludes - #3321

Merged
jamescrosswell merged 19 commits into
mainfrom
inapp-regex
May 6, 2024
Merged

jamescrosswell merged 19 commits into
mainfrom
inapp-regex

Conversation

@jamescrosswell

Copy link
Copy Markdown
Collaborator

Resolves #3158

@jamescrosswell
jamescrosswell requested a review from vaind April 23, 2024 22:14
bitsandfoxes
bitsandfoxes previously approved these changes Apr 24, 2024

@bitsandfoxes bitsandfoxes left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

That is a really nice QOL improvement.

@bitsandfoxes
bitsandfoxes dismissed their stale review April 24, 2024 09:49

Test don't look happy.

Comment thread src/Sentry/SentryStackFrame.cs Outdated
@vaind vaind mentioned this pull request Apr 26, 2024
Comment thread src/Sentry/PrefixOrRegexPattern.cs Outdated
Comment thread src/Sentry.Profiling/SampleProfileBuilder.cs Outdated
Comment thread src/Sentry/SentryOptions.cs Outdated
Comment thread src/Sentry/SentryStackFrame.cs Outdated
@jamescrosswell
jamescrosswell requested a review from vaind April 29, 2024 21:01

@vaind vaind left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It's a bit more complicated solution than what I had in mind in my proposal (albeit more flexible too, I guess), but as long as the performance is OK, I'm fine with that. Have you checked the original vs current version in benchmarks manually? Can you please post the results?

@jamescrosswell

jamescrosswell commented May 6, 2024

Copy link
Copy Markdown
Collaborator Author

Have you checked the original vs current version in benchmarks manually? Can you please post the results?

@vaind the newer code was actually significantly slower (~40%) so I've tightened it up a bit and now I'm seeing:

main branch

Method N Mean Error StdDev Allocated
ConfigureAppFrame 1000 110.5 us 2.16 us 3.02 us 133.77 KB

inapp-regex branch

Method N Mean Error StdDev Allocated
ConfigureAppFrame 1000 62.92 us 1.249 us 3.064 us 976 B

Which is cool... now it's way faster and allocates way less memory 🥳 I think most of those gains were simply getting rid of Linq, although I did get rid of another lambda as well.

@vaind vaind left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

great effort & thanks for the benchmarks

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.

Allow regular expressions in the InAppIncludes and InAppExcludes

4 participants