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

[Rich text editor] Add error tracking for rich text editor #7695

Merged
merged 5 commits into from
Dec 8, 2022
Merged

[Rich text editor] Add error tracking for rich text editor #7695

merged 5 commits into from
Dec 8, 2022

Conversation

jonnyandrew
Copy link
Contributor

Type of change

  • Feature
  • Bugfix
  • Technical
  • Other :

Content

Add Sentry error tracking for internal rich text editor errors.

Motivation and context

https://element-io.atlassian.net/browse/PSU-953

Screenshots / GIFs

See example error tracked in Sentry.

Tests

  • Force an exception from the rich text editor.
  • Check the exception is logged to Sentry.

Tested devices

  • Physical
  • Emulator
  • OS version(s): Android 13

Checklist

@ElementBot
Copy link

Warnings
⚠️

Please add a changelog. See instructions here

Generated by 🚫 dangerJS against 7ccadf3

@jonnyandrew jonnyandrew force-pushed the feat/PSU-953-rich-text-editor-error-tracking branch from 7ccadf3 to 9340eed Compare December 2, 2022 16:41
@ElementBot
Copy link

Warnings
⚠️

Please add a changelog. See instructions here

Generated by 🚫 dangerJS against 9340eed

@jonnyandrew jonnyandrew force-pushed the feat/PSU-953-rich-text-editor-error-tracking branch from 9340eed to 08d5975 Compare December 2, 2022 16:49
@jonnyandrew jonnyandrew requested review from a team, bmarty and amitkma and removed request for a team December 2, 2022 17:47
@jonnyandrew jonnyandrew force-pushed the feat/PSU-953-rich-text-editor-error-tracking branch from 08d5975 to 275f4e2 Compare December 5, 2022 10:05
@jonnyandrew jonnyandrew marked this pull request as ready for review December 5, 2022 10:06
bmarty
bmarty approved these changes Dec 5, 2022
View reviewed changes
Copy link
Member

@bmarty bmarty left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

bmarty
bmarty reviewed Dec 5, 2022
View reviewed changes
Copy link
Member

@bmarty bmarty left a comment

Choose a reason for hiding this comment

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

Actually, I have a concern about user consent, I will comment.

bmarty
bmarty reviewed Dec 5, 2022
View reviewed changes
vector/src/main/java/im/vector/app/features/analytics/impl/DefaultVectorAnalytics.kt Outdated
analyticsConfig: AnalyticsConfig,
private val analyticsStore: AnalyticsStore,
private val lateInitUserPropertiesFactory: LateInitUserPropertiesFactory,
@NamedGlobalScope private val globalScope: CoroutineScope
) : VectorAnalytics {
) : VectorAnalytics, ErrorTracker by sentryAnalytics {
Copy link
Member

Choose a reason for hiding this comment

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

Should we replace ErrorTracker by sentryAnalytics by actual implementation in order to check for user consent in this class, before sending to sentry? This is maybe not necessary, if Sentry does nothing when it is not enabled, but better to double check.

    override fun trackError(throwable: Throwable) {
        sentryAnalytics
                .takeIf { userConsent == true }
                ?.trackError(throwable)
    }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, and I think it's nice to make this explicit.

167a10e

Copy link
Contributor

@amitkma amitkma Dec 5, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

We don't need to check for userConsent because Sentry is not enabled if user has not consented. So any call to Sentry.captureException will do nothing. But yeah, we can keep it explicit.

bmarty
bmarty reviewed Dec 5, 2022
View reviewed changes
vector/src/main/java/im/vector/app/features/analytics/impl/DefaultVectorAnalytics.kt Outdated
analyticsConfig: AnalyticsConfig,
private val analyticsStore: AnalyticsStore,
private val lateInitUserPropertiesFactory: LateInitUserPropertiesFactory,
@NamedGlobalScope private val globalScope: CoroutineScope
) : VectorAnalytics {
) : VectorAnalytics, ErrorTracker {
Copy link
Member

Choose a reason for hiding this comment

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

Since VectorAnalytics extends ErrorTracker, there is no need to mention ErrorTracker again here.

Copy link
Contributor Author

@jonnyandrew jonnyandrew Dec 6, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

83ddf4a

@jonnyandrew jonnyandrew force-pushed the feat/PSU-953-rich-text-editor-error-tracking branch from 7323424 to 83ddf4a Compare December 6, 2022 09:33
@sonarcloud
Copy link

sonarcloud bot commented Dec 8, 2022

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

46.7% 46.7% Coverage
0.0% 0.0% Duplication

@jonnyandrew jonnyandrew merged commit de18f37 into develop Dec 8, 2022
@jonnyandrew jonnyandrew deleted the feat/PSU-953-rich-text-editor-error-tracking branch December 8, 2022 11:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ElementBot ElementBot ElementBot left review comments

@bmarty bmarty bmarty approved these changes

@amitkma amitkma Awaiting requested review from amitkma

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jonnyandrew @ElementBot @bmarty @amitkma