getsentry / sentry-dart-plugin

A Dart Build Plugin that uploads debug symbols for Android, iOS/macOS and source maps for Web to Sentry via sentry-cli
MIT License
66 stars 29 forks source link

Flutter Web sources uploaded even when setting `upload_sources=false` #246

Closed minamax1994 closed 2 months ago

minamax1994 commented 2 months ago

Environment

package version 2.1.0 Flutter version 3.13.2

Steps to Reproduce

I'm using the following sentry.properties config:

upload_debug_symbols=false
upload_source_maps=true
upload_sources=false
include_native_sources=false
ignore_missing=true
project=....
org=....
auth_token=....

Expected Result

Plugin should upload only source maps without source code

Actual Result

Both source maps & source code are uploaded (The first bundle in screenshot includes all code files)

Screenshot 2024-07-01 151956

buenaflor commented 2 months ago

Hey thanks for the issue.

Can you confirm that you're getting this log message after running the plugin: includeSources is disabled, not uploading sources?

minamax1994 commented 2 months ago

Hey @buenaflor

Actually I don't get this message at all, here's part of what I get when I run plugin:

sentry config not found in pubspec.yaml

retrieving config from sentry.properties

Your config contains `include_native_sources` which is deprecated. Consider switching to `upload_sources`.

Downloading Sentry CLI 2.27.0 from https://downloads.sentry-cdn.com/sentry-cli/2.27.0/sentry-cli-Linux-x86_64 to .dart_tool/pub/bin/sentry_dart_plugin/sentry-cli

Sentry CLI binary checksum verification passed successfully (hash: 6b31bbd385d436620415305c12ae181c38bdd3a54c243803dc3ff241ee952356).

Sentry CLI downloaded successfully.
☑ reading config values                                                             
☑ validating config values                                                             

uploadNativeSymbols is disabled.
buenaflor commented 2 months ago

can you try removing include_native_sources? it's the deprecated config field for upload_sources. you should only have one.

minamax1994 commented 2 months ago

@buenaflor

Already tried, same issue

sentry config not found in pubspec.yaml

retrieving config from sentry.properties

Downloading Sentry CLI 2.27.0 from https://downloads.sentry-cdn.com/sentry-cli/2.27.0/sentry-cli-Linux-x86_64 to .dart_tool/pub/bin/sentry_dart_plugin/sentry-cli

Sentry CLI binary checksum verification passed successfully (hash: 6b31bbd385d436620415305c12ae181c38bdd3a54c243803dc3ff241ee952356).

Sentry CLI downloaded successfully.
☑ reading config values                                                             
☑ validating config values                                                             

uploadNativeSymbols is disabled.
buenaflor commented 2 months ago

By the way just for clarification, what kind of code files are you referring to? can you give an example what is included that shouldn't be?

minamax1994 commented 2 months ago

@buenaflor All dart files inside "lib" folder in my project

buenaflor commented 2 months ago

interesting, in any case when upload_debug_symbols is false, then upload_sources is automatically false as well, it shouldn't be executed.

I also just confirmed that it works as intended.

I suppose your pubspec.yaml doesn't have any plugin configuration?

buenaflor commented 2 months ago

I think I know why this might happen, could you try adding this to your sentry.properties:

build_path=/absolute/path/build
web_build_path=/absolute/path/build/web

Note that you need the absolute path here

cc @denrase

minamax1994 commented 2 months ago

@buenaflor Thanks for following up.

I suppose your pubspec.yaml doesn't have any plugin configuration? Yes, all configurations are in sentry.properties

I will update build paths and try again then let you know

denrase commented 2 months ago

@buenaflor I examined what the plugin does. When we have upload_source_maps enabled, we upload all files ending in map and js. So sources together with source maps. We also upload dart files. These are two separate sentry-cli calls, one with --ext map --ext js and one with --ext dart.

I build the app with flutter build web --release --source-maps And ran the plugin with flutter packages pub run sentry_dart_plugin

I tried the following things:

Only upload .map files

When doing this, only the minified names were shown.

https://sentry-sdks.sentry.io/issues/5563151299/?project=5428562&query=is%3Aunresolved+issue.priority%3A%5Bhigh%2C+medium%5D&referrer=issue-stream&statsPeriod=1h&stream_index=3

Stack Trace Source Maps
Bildschirmfoto 2024-07-02 um 14 40 07 Bildschirmfoto 2024-07-02 um 14 39 56

Upload .map and .js files

When doing this, the function names were correct in the frames, but sentry was complaining about missing source context dart file.

https://sentry-sdks.sentry.io/issues/5563170067/?project=5428562&query=is%3Aunresolved+issue.priority%3A%5Bhigh%2C+medium%5D&referrer=issue-stream&statsPeriod=1h&stream_index=1

Stack Trace Source Context Source Maps
Bildschirmfoto 2024-07-02 um 14 42 35 Bildschirmfoto 2024-07-02 um 14 44 59 Bildschirmfoto 2024-07-02 um 14 43 53

Upload .map,.js + .dart files

https://sentry-sdks.sentry.io/issues/5563185073/?project=5428562&query=is%3Aunresolved+issue.priority%3A%5Bhigh%2C+medium%5D&referrer=issue-stream&statsPeriod=1h&stream_index=0

Only some flutter/lib source contexts are marked as missing. Stack trace looks the same as in the sample without .dart files.

I'd guess that we would display source context here, if in-app frame detection were working for Flutter web, which is a known limitation.

Stack Trace Source Context Source Maps
Bildschirmfoto 2024-07-02 um 15 01 43 Bildschirmfoto 2024-07-02 um 14 48 35 Bildschirmfoto 2024-07-02 um 14 47 19

Conclusion

It seems that we at least need the .js files to show de-minified stacktraces for Flutter web.

I think we could disable the .dart file upload if configured as disabled here, but i'm missing context why it is there in the first place. WDYT @buenaflor ?

buenaflor commented 2 months ago

i'm missing context why it is there in the first place

Me aswell, as far as I can see it's been there since the creation of the plugin.

If you haven't found anything that depends on it and I think it makes sense to remove it since source context for Flutter web doesn't work (yet?) anyway, because we would technically need to remap the js stackframe to where it happened in the specific line of code in one of the dart files.

So the steps here would be to

I've also noticed that the warnings now always show up in flutter web even if you do everything right, it wasn't like that before - can you confirm? @denrase

minamax1994 commented 2 months ago

Hi @buenaflor

Sorry for late response. I just tested after setting build paths and it seems to work fine.

Also I can confirm that I now get the Source Context error banner, but it's not a critical issue for me.

Thank you.

buenaflor commented 2 months ago

@denrase there is one big difference, there is source context when uploading with the dart files:

image

vs

image

I didn't know we support source context with flutter web and to be honest don't know how it's implemented, I'll check that out.

But it looks like it's automatically uploaded so far because of source context but I have no idea why it wasn't behind the upload_sources flag.

Looks like we should not remove the uploading and just change it to opt-in with upload_sources

denrase commented 2 months ago

Closed by #247