-
-
Notifications
You must be signed in to change notification settings - Fork 207
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
Regular expressions can now be used for InApp includes/excludes #3321
Conversation
...Sentry.Tests/Internals/SentryStackTraceFactoryTests.MethodGeneric_mode=Enhanced.verified.txt
Outdated
Show resolved
Hide resolved
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.
That is a really nice QOL improvement.
test/Sentry.Profiling.Tests/TraceLogProcessorTests.ProfileInfo_Serialization_Works.verified.txt
Show resolved
Hide resolved
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.
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?
@vaind the newer code was actually significantly slower (~40%) so I've tightened it up a bit and now I'm seeing: main branch
inapp-regex branch
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. |
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.
great effort & thanks for the benchmarks
Resolves #3158