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.

Sign up for GitHub

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

Jump to bottom

Fall back to co-located source maps in sourcemaps inject command #1846

Closed
lforst opened this issue Nov 28, 2023 · 6 comments · Fixed by #1850
Closed

Fall back to co-located source maps in sourcemaps inject command #1846

lforst opened this issue Nov 28, 2023 · 6 comments · Fixed by #1850

Comments

@lforst
Copy link
Member

lforst commented Nov 28, 2023

Based on the feedback in getsentry/sentry-javascript#9666 the CLI should fall back to looking for a generated file's source map in the colocated location when

  • the generated file doesn't have a sourceMapURL comment
  • or the source map can't be found at the location that the sourceMapURL comment points to.

An example of "co-location":

/some/folder
- /output/
  - /my-bundle.js
  - /my-bundle.js.map
@brettdh
Copy link
Contributor

brettdh commented Nov 29, 2023 •
edited
Loading

Hi, author of getsentry/sentry-javascript#9666 here 👋

I'm going to start working on a PR for this. If someone already has something underway, please let me know. Thanks!

One question: if the source file contains an external sourcemap URL and a co-located source map file exists, which one should sourcemaps inject prefer? The common sequence of steps I'm thinking of is this:

  1. remix build --sourcemap (or whatever build step you have to generate sourcemaps)
  2. sentry-cli sourcemaps inject
    • Can the CLI reliably determine that the local sourcemap is newer than the remote one?
    • Should it be told to prefer one or the other via flag?
  3. Upload bundles and source maps to hosting (perhaps to different hosts)
    • If sentry-cli chose wrong in the previous step, the source maps in Sentry are not those that were built for the source files that are about to be deployed

@brettdh
Copy link
Contributor

brettdh commented Nov 29, 2023

I think I was under the impression that the CLI would attempt to download the sourcemap if sourceMappingURL was not a local file... but I'm having trouble finding the code that would do that. It looks like I might just be mistaken about that, and that the CLI treats remote URLs just like file paths... producing some pretty weird results when I run with --log-level debug.

For a js bundle containing this line:

//# sourceMappingURL=https://my-s3-bucket.s3.amazonaws.com/static/build/_shared/chunk-GBSSQ3TL.js.map

sentry-cli sourcemaps inject outputs this:

  DEBUG   2023-11-29 17:41:11.552172 -05:00 Sourcemap file
 /Users/brhiggins/path/to/my/project/public/build/_shared/https://my-s3-bucket.s3.amazonaws.com/static/build/_shared/chunk-GBSSQ3TL.js.map not found

which would certainly explain the failure to find the source map! 😅

@lforst
Copy link
Member Author

lforst commented Nov 30, 2023 •
edited
Loading

I don't think the CLI will download files. It will just look for them locally. If you have "fetching source maps" enabled in Sentry we will also try to grab it from wherever the url points to.

@loewenheim
Copy link
Contributor

That's correct, sentry-cli will never download sourcemaps. The missing piece here is trying a co-located sourcemap if the one in the comment can't be found locally.

Also note that your notion of "co-location" can be expanded a bit using the inject::find_matching_paths function:

sentry-cli/src/utils/sourcemaps/inject.rs

Lines 259 to 269 in 791a1f8

/// Returns a list of those paths among `candidate_paths` that differ from `expected_path` in
/// at most one segment (modulo `.` segments).
///
/// The differing segment cannot be the last one (i.e., the filename).
///
/// If `expected_path` occurs among the `candidate_paths`, no other paths will be returned since
/// that is considered a unique best match.
///
/// The intended usecase is finding sourcemaps even if they reside in a different directory; see
/// the `test_find_matching_paths_sourcemaps` test for a minimal example.
pub fn find_matching_paths(candidate_paths: &[String], expected_path: &str) -> Vec<String> {

brettdh added a commit to brettdh/sentry-cli that referenced this issue Nov 30, 2023
@brettdh
fix(sourcemaps): don't attempt to treat remote URL as a local file path
6ef3189 
When a sourceMappingURL is a remote URL (e.g. static storage in an S3 bucket),
`sentry-cli sourcemaps inject` was treating it like a normal file path, attempting
to "normalize" it with the source path, and producing a bogus url such as
`path/to/source/dir/https://some-static-host.example.com/path/to/foo.js.map`,
which of course doesn't exist, and thus the sourcemap isn't found and doesn't have
the debug id injected - even if it's sitting right there in the local directory,
next to its corresponding bundled source file.

With this change, the sourcemap discovery step does not attempt to use the
discovered sourcemap URL if it is a remote URL. Instead, it falls back to guessing
the sourcemap location based on the source filename and its location.

Fixes getsentry#1846.
brettdh added a commit to brettdh/sentry-cli that referenced this issue Nov 30, 2023
@brettdh
fix(sourcemaps): don't attempt to treat remote URL as a local file path
7413eff 
When a sourceMappingURL is a remote URL (e.g. static storage in an S3 bucket),
`sentry-cli sourcemaps inject` was treating it like a normal file path, attempting
to "normalize" it with the source path, and producing a bogus url such as
`path/to/source/dir/https://some-static-host.example.com/path/to/foo.js.map`,
which of course doesn't exist, and thus the sourcemap isn't found and doesn't have
the debug id injected - even if it's sitting right there in the local directory,
next to its corresponding bundled source file.

With this change, the sourcemap discovery step does not attempt to use the
discovered sourcemap URL if it is a remote URL. Instead, it falls back to guessing
the sourcemap location based on the source filename and its location.

I also changed a couple trycmd tests to remove the `+` before the timezone offset,
as it causes the tests to fail when the local timezone is west of UTC.

Fixes getsentry#1846.
@brettdh brettdh mentioned this issue Nov 30, 2023
Merged
brettdh added a commit to brettdh/sentry-cli that referenced this issue Nov 30, 2023
@brettdh
fix(sourcemaps): don't attempt to treat remote URL as a local file path
b27e220 
When a sourceMappingURL is a remote URL (e.g. static storage in an S3 bucket),
`sentry-cli sourcemaps inject` was treating it like a normal file path, attempting
to "normalize" it with the source path, and producing a bogus url such as
`path/to/source/dir/https://some-static-host.example.com/path/to/foo.js.map`,
which of course doesn't exist, and thus the sourcemap isn't found and doesn't have
the debug id injected - even if it's sitting right there in the local directory,
next to its corresponding bundled source file.

With this change, the sourcemap discovery step does not attempt to use the
discovered sourcemap URL if it is a remote URL. Instead, it falls back to guessing
the sourcemap location based on the source filename and its location.

I also changed a couple trycmd tests to remove the `+` before the timezone offset,
as it causes the tests to fail when the local timezone is west of UTC.

Fixes getsentry#1846.
@lforst lforst reopened this Dec 1, 2023
@lforst lforst closed this as completed Dec 1, 2023
@brettdh
Copy link
Contributor

brettdh commented Dec 20, 2023

Hmm, it seems that this issue also occurs if the sourceMappingURL is a URL path (without the https://<domain> part). This is indistinguishable from a file path, though. The logic here should probably be adjusted to fall back to colocation heuristics if the URL is a full URL or a file that can't be found. (It makes me wonder under what circumstances the value of sourceMappingURL will actually be used, but I digress.)

@lforst Better to create a new issue, or reopen this one?

@lforst
Copy link
Member Author

lforst commented Dec 21, 2023

@brettdh Please open a new issue with exact steps to reproduce what is wrong. Thanks!

@brettdh brettdh mentioned this issue Dec 21, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

fix(sourcemaps): don't attempt to treat remote URL as a local file path brettdh/sentry-cli
3 participants
@brettdh @loewenheim @lforst