-
-
Notifications
You must be signed in to change notification settings - Fork 4.5k
Move system symbol lookup into a postgres database. #3022
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
Current coverage is
|
src/sentry/lang/native/symbolizer.py
Outdated
from sentry.lang.native.dsymcache import dsymcache | ||
|
||
|
||
def find_system_symbol(img, instruction_addr): |
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.
Isn't this O(n) for number of frames? with a RT to database? Possibly two queries per frame.
I will try to play with splitting the sentry_dsymsymbol into two tables because lots of symbols are reused but at different addresses. That might cut down storage quite a bit. |
Splitting up the symbols into just the strings and then referencing them from elsewhere does not appear to be a great storage saver. The various indexes get huge in total. I will try a full import over night to see if with more data it improves though. |
@getsentry/api I will aim for getting this in for getsentry by EOW. So probably friday the last. If it's bad, we can easily back it out. The old code runs in addition for now. I will set up a router in a new pull request on getsentry that moves it to a separate DB if we need to. I want to push this out asap because i need to get some other support in (tvos and code) and i don't want to have to do that with the system symbol code path. |
@getsentry/api This is ready for merge as far as I'm concerend. It does not yet remove the other branch for system symbols so we can slowly upload the preprocessed ones. I also want to do those things after this is merged:
|
sdk_name = SDK_MAPPING[info['system_name']] | ||
system_version = tuple(int(x) for x in ( | ||
info['system_version'] + '.0' * 3).split('.')[:3]) | ||
except LookupError: |
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.
TIL LookupError
I'm just going to kill the preparing now since that only worked on postgres anyways and just give up with trying to optimize this. Too much of a hassle with also making it idempotent and not really worth the effort. We don't import often enough. @getsentry/api going to merge and deploy this tomorrow. |
2825f6b
to
0287a7a
Compare
This is ready to merge from where I'm standing. @getsentry/api does anyone want to do a second look over the migrations before I merge this? |
7ddcaf6
to
cc45369
Compare
This is still work in progress but the idea is to move all system symbol lookup into the database. Right now it uses the main database but i want to split this up and maybe later move it into a webservice.
Potential improvements for later are to look up multiple symbols at once but that would work best with a stored procedure.