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
68 stars 28 forks source link

Release from env var used by plugin is not fully customizable #207

Closed momrak closed 7 months ago

momrak commented 8 months ago

Environment

1.7.1

Steps to Reproduce

In my GH action I pass in the commit SHA as the SENTRY_RELEASE env var. When calling the sentry_dart_plugin as a part of the GH workflow it then prints this #15 3.065 Created release f77da8206d0c51aef4d055ebae6a6b90433c82c5@0.0.146+3 So the release has gotten the version and buildnr from pubspec file appended @0.0.146+3 . Is this how it should be ,or am I doing something wrong?

Expected Result

I was expecting the release to be the SHA f77da8206d0c51aef4d055ebae6a6b90433c82c5 only

Actual Result

It is sha with version and buildnr #15 3.065 Created release f77da8206d0c51aef4d055ebae6a6b90433c82c5@0.0.146+3

In this section of the docs https://docs.sentry.io/platforms/flutter/configuration/releases/#bind-the-version on of the recommended ways of dealing with version is this

Commit SHA: If you use a version control system like Git, we recommend using the identifying hash (for example, the commit SHA, da39a3ee5e6b4b0d3255bfef95601890afd80709). You can let Sentry CLI automatically determine this hash for supported version control systems. Learn more in our Sentry CLI documentation.

But it does not seem possible to do this so I am a bit confused. I have perhaps misunderstood how this should be.

momrak commented 8 months ago

Here is a concrete example where you can see the value of the env var and also what is being uploaded as a new release

I will then have to add the versions and builnr when initing the SDK in the app and it just seems strange that the env var is not used directly as is. Is this as intended?

CleanShot 2024-03-07 at 20 29 32@2x

bawahakim commented 8 months ago

I have the same issue. Also unsure if it's the intended behaviour, hopefully not because that makes it hard to match with the release defined in the SDK

Edit: this also happens locally

momrak commented 8 months ago

I tried now passing SENTRY_RELEASE=myappname@313563mysha and then it works. So it seems it should always be this form. If that is correct I think the docs should maybe be updated to make this a bit clearer

bawahakim commented 8 months ago

@momrak I imagine this would work, but then that would mean the release we use at runtime must match this string?

momrak commented 8 months ago

Yes, but if you pass it with --dart-define=SENTRY_RELEASE=myappname@313563mysha it will be set to this automatically. If you run this you can see. Not 100% sure if the dist still has to be set to something, but I think not as there is no +<build> in the release name. This is still unclear to me though

    await SentryFlutter.init(
      (options) {
      print(options.release); // -> myappname@313563mysha
        return options
          ..dsn = config.sentryDsn
          ..environment = config.environment
      },
    );
  }

But I still think the docs should better explain this if it is the case. Because when setting this env var you don't automatically expect @<version>+<build> to just be added. Or at least I did not.

momrak commented 8 months ago

I noticed now that the uploaded release also includes the dist, so you probably then need to add that to the initial options.. If setting the dist is enough then, or if you need to add the dist to the release in order to get it to behave correctly I am not sure. This is all a bit confusing CleanShot 2024-03-12 at 10 42 27

buenaflor commented 8 months ago

Seems like my comment wasn't posted πŸ˜“

we should also support commit hashes as per doc you linked

bawahakim commented 8 months ago

Thanks @momrak . In this case, how does the release look like when you look at Sentry issues? I'd like for it show as the version number only, not version@build or some other enforced @ version

momrak commented 8 months ago

I updated my project now so setting the var to be SENTRY_RELEASE=<project-name>@<gh-sha> That now creates the following. So seems it adds the dist automatically.

#15 13.07 > Release: <project-name>@475119530cc071f08fcd25f5d7807360f3b0203f+3
#15 13.07 > Dist: 3

Then in my main.dart I need to update the release that is automatically set from options do include the buildnr.

    await SentryFlutter.init(
      (options) {
        final release = '${options.release}+${packageInfo.buildNumber}';

        return options
          ..release = release
          ..dist = packageInfo.buildNumber
          ..dsn = config.sentryDsn
          ..environment = config.environment
      },
    );

If I remember correctly it was something like when I had SENTRY_RELEASE=<gh-sha> it would end up as <gh-sha>@<version>+<dist> so the @ and version and dist got added automatically

bawahakim commented 8 months ago

Thanks @momrak ! I think it actually works fine for the release even without adding the +. What I got confused is that my stack trace were actually shown properly, but it includes irrelevant stacks, and also the title is obfuscated, so I thought the upload did not work.

But I see the source file name and code line of the error, and it seems the obfuscated title is a known issue https://github.com/getsentry/sentry-dart-plugin/issues/154

momrak commented 8 months ago

Aha, I will try that. So only setting the dist like this and that should work?

    await SentryFlutter.init(
      (options) {
        return options
          ..dist = packageInfo.buildNumber
          ..dsn = config.sentryDsn
          ..environment = config.environment
      },
    );

Yes, hoping for some fix on the obfuscated title 🀞

bawahakim commented 8 months ago

For me it seems I don't have to pass in the dist, it picks it up from the build number already. I only explictly pass the release as I need it (I use semver, so my release is X.Y.Z) and set SENTRY_RELEASE to the same X.Y.Z. It seems this still picks up the symbols properly, even if this plugin gives a different release name. More testing is needed though, will update if my future findings contradicts this.

denrase commented 8 months ago

I have gone through the discussion and tried to implement everything in https://github.com/getsentry/sentry-dart-plugin/pull/217. Please check the description on the new behaviour and give us feedback if this solves your issues. πŸ™‡β€β™‚οΈ

momrak commented 8 months ago

This looks good πŸ˜„ Some version of the summary of #217 would be nice to get into the docs as well to explain this

blaugold commented 7 months ago

217 Looks good to me as well!

buenaflor commented 7 months ago

PR is merged, this will be part of the next release