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

feat: Migrate from bugsnag to sentry #1471

Merged
merged 9 commits into from
Jan 11, 2021
Merged

Conversation

adamfilipow92
Copy link
Contributor

@adamfilipow92 adamfilipow92 commented Jan 7, 2021

Fixes #1318

Test Plan

How do we know the code works?

  1. Flank should display session id below revision
  2. Should save session id to session_id.txt in matrix path
  3. Should upload session_id.txt when disable-results-upload not set
  4. Unit & Integration tests should pass

Checklist

  • Save and upload session id
  • Unit tested
  • Integration tests updated

@adamfilipow92 adamfilipow92 self-assigned this Jan 7, 2021
@github-actions
Copy link
Contributor

github-actions bot commented Jan 7, 2021

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

@github-actions
Copy link
Contributor

github-actions bot commented Jan 7, 2021

Timestamp: 2021-01-11 08:11:24
Buildscan url for ubuntu-workflow run 477032837
https://gradle.com/s/ojpo4zixos454

@@ -2,6 +2,9 @@ object Versions {
// https://github.com/bugsnag/bugsnag-java/releases
const val BUGSNAG = "3.6.2"
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we remove it?

@adamfilipow92
Copy link
Contributor Author

I have read the CLA Document and I hereby sign the CLA

@adamfilipow92
Copy link
Contributor Author

recheck

UUID.randomUUID().toString()
}

fun setCrashReportTag(key: String, value: String) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This could be a setCrashReportTag(vararg tags: Pair<String, String>)

private const val ANALYTICS_FILE = "analytics-uuid"
private const val DISABLED = "DISABLED"

enum class CrashReportTag(val tagName: String) {
Copy link
Contributor

Choose a reason for hiding this comment

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

If we are not iterating or switching, the object with constant values will fit better than the enum.

@adamfilipow92 adamfilipow92 marked this pull request as ready for review January 11, 2021 00:42
@bootstraponline bootstraponline force-pushed the 1318-from-bugsnag-to-sentry branch 2 times, most recently from 4e148d0 to 410310a Compare January 11, 2021 06:47
@bootstraponline bootstraponline force-pushed the 1318-from-bugsnag-to-sentry branch from 410310a to 1a9bf76 Compare January 11, 2021 07:05
else -> initializeCrashReportWrapper()
}

private fun analyticsFileExistAndIsDisabled(rootPath: String) =
Copy link
Contributor

Choose a reason for hiding this comment

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

Hm...this name is not fully self-explanatory (for me). The logic here is to check if a user had disabled google analytics, flank should respect those settings.
How about isGoogleAnalyticsDisabled? Maybe not exactly the same but something explains why we check that file

Copy link
Contributor Author

Choose a reason for hiding this comment

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

agree

@@ -0,0 +1,60 @@
package ftl.util
Copy link
Contributor

Choose a reason for hiding this comment

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

Small typo in the file name CreashReporter -> CrashReporter

@@ -101,6 +103,14 @@ object GcStorage {
)
}

fun IArgs.uploadSessionId() = takeUnless { disableResultsUpload }?.let {
upload(
file = Path.of(localResultDir, SESSION_ID_FILE).toString(),
Copy link
Contributor

Choose a reason for hiding this comment

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

We use Paths in other places in the project. Your implementation is not wrong! I am just wondering if it is possible to use Paths here as well

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed! Thanks! 👍

@mergify mergify bot merged commit 9bf7b23 into master Jan 11, 2021
@mergify mergify bot deleted the 1318-from-bugsnag-to-sentry branch January 11, 2021 14:35
@github-actions github-actions bot locked and limited conversation to collaborators Jan 11, 2021
@zuziaka zuziaka added this to the Sprint -1 milestone Jan 19, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Evaluate moving to Sentry from BugSnag
5 participants