feat(difs): Add dif candidates info to module list #309

flub · 2020-11-27T13:34:44Z

This adds new information about which DIF candidates were considered
to be used for modules. So far only part of all the information we
want to expose is present.

sentry snapshot updates: getsentry/sentry#22395

This adds new information about which DIF candidates were considered to be used for modules. So far only part of all the information we want to expose is present.

- Describe things properly. - Make sure the type names make sense and are sanely used. - Optimise some Vec sharing. - Scope future work better.

jan-auer

Let's please put all the candidate information behind a config flag. That is, we can compute all the information internally, but only serialize it when the flag is given. This allows us to iterate on this feature and make releases without impacting any users -- including Sentry.

src/actors/cficaches.rs

src/actors/objects/mod.rs

src/types.rs

Co-authored-by: Jan Michael Auer <mail@jauer.org>

It is possible for the meta (and data) caches to return transient errors. This extends the diagnostics built up with this information as well.

Providing details on malformed requires a lot more refactoring, probably not for this PR.

At least for now, there's a followup work note on the doc.

src/types.rs

That says a bit better what it actually is.

Wouldn't it be nice if this would be statically enforced? Removing deserialisation was also an option, but we'll need it later.

Adding details will require a lot of caching refactoring.

flub · 2020-12-02T13:58:42Z

This PR only covers the download info, the debug and unwind info will follow in followup PRs.

github-actions · 2020-12-02T14:09:42Z

	Warnings
⚠️	Snapshot changes likely affect Sentry tests. Please check the symbolicator test suite in Sentry and update snapshots as needed.

Instructions for snapshot changes

Sentry runs a symbolicator integration test suite located at tests/symbolicator/. Changes in this PR will likely result in snapshot diffs in Sentry, which will break the master branch and in-progress PRs.

Follow these steps to update snapshots in Sentry:

Check out latest Sentry master and enable the virtualenv.
Stop the symbolicator devservice using sentry devservices down symbolicator.
Run your development symbolicator on port 3021.
Export SENTRY_SNAPSHOTS_WRITEBACK=1 and run symbolicator tests with pytest.
Review snapshot changes locally, then create a PR to Sentry.
Merge the Symbolicator PR, then merge the Sentry PR.

Generated by 🚫 dangerJS against 6c64d66

Symbolicator snapshots need updating for the new download info available from getsentry/symbolicator#309.

src/actors/symbolication.rs

src/actors/symcaches.rs

src/types.rs

Swatinem

lets do it. Since you will solve some of the things I mentioned in the followup

Co-authored-by: Arpad Borsos <arpad.borsos@sentry.io>

flub · 2020-12-09T12:03:46Z

@Swatinem @jan-auer PTAL, I realised it didn't include a config setting to disable it yet. Last commit adds this. No other changes.

src/actors/symbolication.rs

src/config.rs

jan-auer

After discussing offline, two final change requests:

Please move the flag to the request struct (for all request types)
Please default it to off

This requires the request to opt into the dif candidates info. It introduces a new options field that could be extended further in the future.

flub · 2020-12-15T12:12:40Z

Apologies for the force-pushing. I hard resetted the last config commit without realising that's probably not the best way for the PR. Anyway, the DIF info is now only enabled if in the request as can be seen in the integration tests.

This also means the matching PR for the sentry snapshot tests doesn't need to happen as those responses should not change at all.

untitaker · 2020-12-16T09:33:06Z

src/utils/multipart.rs

@@ -46,3 +47,11 @@ pub fn read_multipart_sources(
            .and_then(|data| Ok(serde_json::from_slice(&data)?)),
    )
 }
+
+pub fn read_multipart_request_data(


This + read_multipart_sources can be one generic fn

Yes, that would seem sane but it appears I'm not smart enough to understand serde's lifetimes. I'm just living with this for now instead of wasting loads of time. I'd appreciate any hints though.

src/endpoints/symbolicate.rs

jan-auer

I clicked approve too quickly again.

src/endpoints/applecrashreport.rs

jan-auer

Now for real.

Floris Bruynooghe added 2 commits November 27, 2020 14:31

feat(difs): Add dif candidates info to module list

fbdb626

This adds new information about which DIF candidates were considered to be used for modules. So far only part of all the information we want to expose is present.

Cleanup names, descriptions and todos

c90ef7c

- Describe things properly. - Make sure the type names make sense and are sanely used. - Optimise some Vec sharing. - Scope future work better.

jan-auer reviewed Nov 30, 2020

View reviewed changes

Floris Bruynooghe and others added 4 commits November 30, 2020 11:05

Some renames from PR feedback

44e005d

Follow through with renaming

6607a52

Apply suggestions from code review

1a2dc08

Co-authored-by: Jan Michael Auer <mail@jauer.org>

Provide the candidates info for errors while fetching too

321025a

It is possible for the meta (and data) caches to return transient errors. This extends the diagnostics built up with this information as well.

flub requested review from jan-auer and Swatinem November 30, 2020 16:43

Floris Bruynooghe added 4 commits December 1, 2020 17:10

Update the response format and accept the new tests

f05a9b8

Remove this, it completely breaks

38a2310

Basic malformed implementation

b5bdcb4

Providing details on malformed requires a lot more refactoring, probably not for this PR.

Do not have details for malformed

468082c

At least for now, there's a followup work note on the doc.

flub commented Dec 2, 2020

View reviewed changes

src/types.rs Show resolved Hide resolved

Floris Bruynooghe added 6 commits December 2, 2020 12:12

Horrible horrible integration test fixing

c22b92c

Do not serialise candidates if there are none

f18997e

Rename ObjectFile into ObjectHandle

f73cc8a

That says a bit better what it actually is.

Fix doc link

33af0a8

Ensure this can be deserialised as well

840d71a

Wouldn't it be nice if this would be statically enforced? Removing deserialisation was also an option, but we'll need it later.

Remove details from this malformed status too

d347430

Adding details will require a lot of caching refactoring.

Add a changelog

ed0c479

flub marked this pull request as ready for review December 2, 2020 14:02

flub requested a review from a team December 2, 2020 14:02

Merge branch 'master' into feat/dif-info

cee6e77

Merge branch 'master' into feat/dif-info

ca02ea7

flub pushed a commit to getsentry/sentry that referenced this pull request Dec 2, 2020

test(symbolicator): Update snapshots

6a78e7e

Symbolicator snapshots need updating for the new download info available from getsentry/symbolicator#309.

flub mentioned this pull request Dec 2, 2020

test(symbolicator): Update snapshots getsentry/sentry#22395

Closed

Swatinem reviewed Dec 4, 2020

View reviewed changes

src/actors/symbolication.rs Show resolved Hide resolved

src/actors/symcaches.rs Show resolved Hide resolved

src/types.rs Outdated Show resolved Hide resolved

Swatinem approved these changes Dec 7, 2020

View reviewed changes

jan-auer approved these changes Dec 7, 2020

View reviewed changes

typo

cd70fe2

Co-authored-by: Arpad Borsos <arpad.borsos@sentry.io>

flub requested review from Swatinem and jan-auer December 9, 2020 12:09

flub commented Dec 9, 2020

View reviewed changes

src/actors/symbolication.rs Show resolved Hide resolved

jan-auer approved these changes Dec 14, 2020

View reviewed changes

jan-auer reviewed Dec 14, 2020

View reviewed changes

src/config.rs Outdated Show resolved Hide resolved

jan-auer requested changes Dec 14, 2020

View reviewed changes

Make the dif info opt-in via the request

458e4f8

This requires the request to opt into the dif candidates info. It introduces a new options field that could be extended further in the future.

flub force-pushed the feat/dif-info branch from 401b54f to 458e4f8 Compare December 15, 2020 12:10

flub requested review from jan-auer and untitaker December 15, 2020 12:12

Update gcs test as well

0f8c977

untitaker approved these changes Dec 16, 2020

View reviewed changes

untitaker reviewed Dec 16, 2020

View reviewed changes

jan-auer approved these changes Dec 16, 2020

View reviewed changes

src/endpoints/symbolicate.rs Outdated Show resolved Hide resolved

untitaker mentioned this pull request Dec 16, 2020

feat(native): Expose dif candidates to frontend getsentry/sentry#22719

Merged

jan-auer requested changes Dec 16, 2020

View reviewed changes

src/endpoints/applecrashreport.rs Outdated Show resolved Hide resolved

Floris Bruynooghe added 2 commits December 16, 2020 15:40

Muck about with json

15f1e69

Remove unneeded option

6c64d66

flub requested a review from jan-auer December 16, 2020 14:42

jan-auer approved these changes Dec 16, 2020

View reviewed changes

flub merged commit eb9b262 into master Dec 16, 2020

flub deleted the feat/dif-info branch December 16, 2020 14:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(difs): Add dif candidates info to module list #309

feat(difs): Add dif candidates info to module list #309

flub commented Nov 27, 2020 •

edited

Loading

jan-auer left a comment

flub commented Dec 2, 2020

github-actions bot commented Dec 2, 2020 •

edited

Loading

Swatinem left a comment

flub commented Dec 9, 2020

jan-auer left a comment

flub commented Dec 15, 2020

untitaker Dec 16, 2020

flub Dec 16, 2020

jan-auer left a comment

jan-auer left a comment

feat(difs): Add dif candidates info to module list #309

feat(difs): Add dif candidates info to module list #309

Conversation

flub commented Nov 27, 2020 • edited Loading

jan-auer left a comment

Choose a reason for hiding this comment

flub commented Dec 2, 2020

github-actions bot commented Dec 2, 2020 • edited Loading

Swatinem left a comment

Choose a reason for hiding this comment

flub commented Dec 9, 2020

jan-auer left a comment

Choose a reason for hiding this comment

flub commented Dec 15, 2020

untitaker Dec 16, 2020

Choose a reason for hiding this comment

flub Dec 16, 2020

Choose a reason for hiding this comment

jan-auer left a comment

Choose a reason for hiding this comment

jan-auer left a comment

Choose a reason for hiding this comment

flub commented Nov 27, 2020 •

edited

Loading

github-actions bot commented Dec 2, 2020 •

edited

Loading