Remove CrashLoggingDataProvider.releaseName

[crazytonyli]

The same change was made to the iOS Tracks library in Automattic/Automattic-Tracks-iOS#267. The gist is we'd want to use the default release name, whose format is <app-id>@<version>+<build>.

See pdnsEh-1mT-p2 and p1710798607285959-slack-C6H8C3G23

[oguzkocer]

@crazytonyli Since this library doesn't have major/minor releases, I suggest updating the clients before merging this to avoid breaking any of the clients.

[crazytonyli]

@oguzkocer Thanks for the heads up! I'll look into updating the apps now.

[crazytonyli]

@oguzkocer I have updated the apps. See the linked PR above ⏫

Since this library doesn't have major/minor releases

Do you mean the library releases don't follow semantic versioning? Once this PR is merged, should I publish the new release as 4.0.0 or 3.5.1?

[wzieba]

I thought we generally follow semantic versioning here. I guess it should be 4.0.0. Or if we version this differently, what's the rule @oguzkocer ?

---

About the PR - I guess it'll make some mess in Sentry release dashboards. In WooCommerce Android we have this logic:

which prevents from adding multiple "PR releases", as it is in WordPress/Jetpack app

WDYT about keeping the releaseName and request build so we can join and prepare <app-id>@<version>+<build> ourselves? This would still allow us to have a common "PR release" so we won't pollute the Sentry dashboard.

[wzieba]

Or maybe even easier way: keep val releaseName: String but make it nullable. Then update WooCommerce Android logic to:

override val releaseName: String? = if (buildConfig.debug) {
        DEBUG_RELEASE_NAME
    } else {
        // delegate preparing release name to Sentry SDK
        null
    }

[oguzkocer]

Do you mean the library releases don't follow semantic versioning? Once this PR is merged, should I publish the new release as 4.0.0 or 3.5.1?

Unfortunately we don't follow semantic versioning as closely as we should. I'd tag this as 4.0.0 because we know it has breaking changes. Btw, if it didn't, it'd be 3.6.0 since the z in x.y.z is reserved for patches/hotfixes.

[oguzkocer]

Or maybe even easier way: keep val releaseName: String but make it nullable.

@wzieba I am not a fan of overloading the meaning of null value like this in a library. If we want to make it optional, let's use a sealed class, so we can tell how it'll be handled without having to look up the implementation. Something like:

sealed class SentryReleaseName {
    class SetByApplication(val releaseName: String): SentryReleaseName()
    data object SetByTracksLibrary : SentryReleaseName()
}

[wzieba]

Sure, good idea @oguzkocer 👍 We have a similar approach with PerformanceMonitoringConfig

Automattic-Tracks-Android/AutomatticTracks/src/main/java/com/automattic/android/tracks/crashlogging/CrashLoggingDataProvider.kt

Lines 83 to 104 in 7f6f941

	sealed class PerformanceMonitoringConfig {
	object Disabled : PerformanceMonitoringConfig()
	data class Enabled(
	/**
	* Provides sample rate for performance monitoring. Indicates how often do we measure performance.
	* Has to be between 0 and 1.
	*/
	val sampleRate: Double,
	/**
	* Provides sample rate for recording profiles.
	* Indicates how often do we record profile for a performance transaction.
	* Mind that this value is **relative** to [sampleRate] value.
	* Has to be between 0 and 1.
	*/
	val profilesSampleRate: Double = 0.0
	) : PerformanceMonitoringConfig() {
	init {
	assert(sampleRate in 0.0..1.0)
	assert(profilesSampleRate in 0.0..1.0)
	}
	}

[wzieba]

One thing I'd change to this suggestion is to make it SDK-agnostic, as the rest of the crashlogging module, I mean SentryReleaseName -> ReleaseName or CrashLoggingReleaseName

[crazytonyli]

@wzieba Good catch regarding the PR releases.

In WooCommerce Android we have this logic [...] which prevents from adding multiple "PR releases", as it is in WordPress/Jetpack app

I noticed the WC iOS app has PR releases in Sentry too. Would it be okay to make the Android app do the same?

[wzieba]

I noticed the WC iOS app has PR releases in Sentry too. Would it be okay to make the Android app do the same?

I'm not sure. What I'd like to keep is to not create releases for PRs, but keep them all in artificial "debug" release as the logic from WC Android goes. The PR releases don't seem to have any value for product teams, and they just clutter the dashboard. I'd vote more to keep this kind of logic in clients instead:

[wzieba]

I've prepared the suggestion in form of PR, #206 WDYT @crazytonyli ?

[wzieba]

@oguzkocer

Unfortunately we don't follow semantic versioning as closely as we should.

That's true 😕. I think binary compatibility validator could help us in this matter for crashlogging package at least, by raising awareness of the API contract. I added #207 to track it.

[crazytonyli]

The PR releases don't seem to have any value for product teams, and they just clutter the dashboard.

I agree. Finding crash report is probably the only value in the PR releases. And having one release for all PRs is sufficient for finding crash reports.

I don't really have a strong opinion on either approach. But I do feel like there is value in keeping it consistent across all apps on both platforms.

#206 looks good to me. What do you think rebasing that PR with the trunk branch and I'll close this one?

[wzieba]

But I do feel like there is value in keeping it consistent across all apps on both platforms.

Good idea 👍 I'll come up with a PR to iOS Tracks.

#206 looks good to me. What do you think rebasing that PR with the trunk branch and I'll close this one?

Sure thing, thanks!

[crazytonyli]

Closing in favor of #206.