Skip to content
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

fix: Properly mask Portable PDB Age for symstore/SSQP lookups #888

Merged
merged 2 commits into from
Oct 19, 2022

Conversation

Swatinem
Copy link
Member

Portable PDB lookups via the SSQP scheme say that the Age portion of the ID needs to be replaced with u32::MAX.

See https://github.com/dotnet/symstore/blob/main/docs/specs/SSQP_Key_Conventions.md#portable-pdb-signature

This allows us to properly look up Portable PDB files on the upstream microsoft symbol server.
For example, http://msdl.microsoft.com/download/symbols/microsoft.aspnetcore.routing.pdb/fbd5dfdfa5a44e09a75862fcbb8c914aFFFFFFFF/microsoft.aspnetcore.routing.pdb might be such a valid file which matches a stack frame from https://sentry.io/organizations/sentry-sdks/issues/2442781715/events/a4f4c59a7d1a4b06bd98c0d1dcfa7e8c/?project=5428537

Portable PDB lookups via the SSQP scheme say that the `Age` portion of the ID needs to be replaced with u32::MAX.

See https://github.com/dotnet/symstore/blob/main/docs/specs/SSQP_Key_Conventions.md#portable-pdb-signature

This allows us to properly look up Portable PDB files on the upstream microsoft symbol server.
@Swatinem Swatinem requested a review from a team October 19, 2022 12:26
@github-actions
Copy link
Contributor

github-actions bot commented Oct 19, 2022

Warnings
⚠️ Snapshot changes likely affect Sentry tests. If the Sentry-Symbolicator Integration Tests in CI are failing for your PR, 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:

  1. Check out latest Sentry master and enable the virtualenv.
  2. Enable symbolicator (symbolicator: true) in sentry via ~/.sentry/config.yml.
  3. Make sure your other devservices are running via sentry devservices up --exclude symbolicator. If
    they're already running, stop symbolicator with sentry devservices down symbolicator. You want to use your
    own development symbolicator to update the snapshots.
  4. Run your development symbolicator on port 3021, or whatever port symbolicator is configured to use
    in ~/.sentry/config.yml.
  5. Export SENTRY_SNAPSHOTS_WRITEBACK=1 to automatically update the existing snapshots with your new
    results and run symbolicator tests with pytest (pytest tests/symbolicator).
  6. Review snapshot changes locally, then create a PR to Sentry.
  7. Merge the Symbolicator PR, then merge the Sentry PR.

Generated by 🚫 dangerJS against df237bb

@@ -38,7 +38,7 @@ sources:
type: http
url: https://msdl.microsoft.com/download/symbols/
layout: { type: "symstore" }
Copy link
Member Author

Choose a reason for hiding this comment

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

tbh, I have no idea why this is symstore and not ssqp. The only difference seems to be the casing, and the upstream microsoft server seems to be case insensitive anyway as far as I have tried.

@codecov-commenter
Copy link

codecov-commenter commented Oct 19, 2022

Codecov Report

Base: 70.27% // Head: 70.28% // Increases project coverage by +0.00% 🎉

Coverage data is based on head (df237bb) compared to base (1370da5).
Patch coverage: 80.00% of modified lines in pull request are covered.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #888   +/-   ##
=======================================
  Coverage   70.27%   70.28%           
=======================================
  Files          59       59           
  Lines       11493    11496    +3     
=======================================
+ Hits         8077     8080    +3     
  Misses       3416     3416           
Impacted Files Coverage Δ
crates/symbolicator/src/utils/paths.rs 75.59% <80.00%> (+0.14%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

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.

3 participants