RUM-2907 Allow setting custom error fingerprint by fuzzybinary · Pull Request #1911 · DataDog/dd-sdk-android

fuzzybinary · 2024-03-12T19:04:48Z

What does this PR do?

This sets up attributes that are automatically transferred to the Error objects for Logs and RUM.

For logs, the attribute is LogAttributes.ERROR_FINGERPRINT and sets the fingerprint in the LogEvent.Error.

For RUM, the attribute is RumAttributes.ERROR_FINGERPRINT and sets the fingerprint member in ErrorEvent.Error

Also fixes RUM-2209 by adding the LogEvent.Error.fingerprint member

Review checklist (to be filled by reviewers)

Feature or bugfix MUST have appropriate tests (unit, integration, e2e)
Make sure you discussed the feature or bugfix with the maintaining team in an Issue
Make sure each commit and the PR mention the Issue number (cf the CONTRIBUTING doc)

This sets up an attribute, `LogAttributes.ERROR_FINGERPRINT` that is automatically transferred to the `Error` object in `LogEvent`s. Also fixes RUM-2209 by adding the `LogEvent.Error.fingerprint` member

codecov-commenter · 2024-03-12T21:40:37Z

Codecov Report

Merging #1911 (68b3fc8) into develop (f46eb76) will decrease coverage by 0.02%.
Report is 6 commits behind head on develop.
The diff coverage is 100.00%.

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #1911      +/-   ##
===========================================
- Coverage    83.38%   83.36%   -0.02%     
===========================================
  Files          477      477              
  Lines        16922    16971      +49     
  Branches      2514     2525      +11     
===========================================
+ Hits         14109    14147      +38     
- Misses        2120     2125       +5     
- Partials       693      699       +6

Files	Coverage Δ
...in/kotlin/com/datadog/android/log/LogAttributes.kt	`0.00% <ø> (ø)`
...android/log/internal/domain/DatadogLogGenerator.kt	`98.06% <100.00%> (+0.09%)`	⬆️
...in/kotlin/com/datadog/android/rum/RumAttributes.kt	`0.00% <ø> (ø)`
.../main/kotlin/com/datadog/android/rum/RumMonitor.kt	`28.57% <ø> (ø)`
...roid/rum/internal/domain/scope/RumResourceScope.kt	`94.87% <100.00%> (+0.78%)`	⬆️
.../android/rum/internal/domain/scope/RumViewScope.kt	`93.29% <100.00%> (+0.02%)`	⬆️

... and 26 files with indirect coverage changes

0xnm

Looks good, but I think it is missing the following:

We need also implementation for RUM. Is it going to be added in another PR?
How we are going to let know the customers that they can use this attribute? Are we going to update some documentation?

fuzzybinary · 2024-03-13T12:13:06Z

We need also implementation for RUM. Is it going to be added in another PR?

I'll go back and do it for this one.

How we are going to let know the customers that they can use this attribute? Are we going to update some documentation?

That was my plan. Do you think there's a better way?

0xnm · 2024-03-13T12:27:19Z

That was my plan. Do you think there's a better way?

probably updating documentation of related API methods will be enough.

0xnm · 2024-03-13T15:25:51Z

    const val ERROR_DATABASE_PATH: String = "error.database.path"

+    /**
+     * Specifies a custom error fingerprint for the supplied.


Suggested change

* Specifies a custom error fingerprint for the supplied.

* Specifies a custom error fingerprint for the supplied error.

Also let's add a reference to RumAttributes.ERROR_FINGERPRINT in the RumMonitor APIs descriptions where applicable.

0xnm · 2024-03-13T15:28:50Z

        val updatedAttributes = addExtraAttributes(event.attributes)
        val isFatal = updatedAttributes
            .remove(RumAttributes.INTERNAL_ERROR_IS_CRASH) as? Boolean == true || event.isFatal
+        val errorFingerprint = updatedAttributes.remove(RumAttributes.ERROR_FINGERPRINT) as? String


Should we support this attribute in the RumResourceScope as well? 🤔 there is a possibility in API to stop resource with error.

I don't have an answer for that, just thinking.

It's a good thought, and I don't have an answer for it either. Are resource errors considered in Error Tracking and thus would they consult the fingerprint? Maybe a question for the ET team.

0xnm · 2024-03-13T15:31:28Z

+            assertThat(firstValue)
+                .apply {
+                    hasMessage(throwableMessage)
+                    hasStackTrace(stacktrace)
+                    hasErrorFingerprint(fingerprint)
+                }


ideally we need to assert all properties of the event as it is done in other tests.

Or at least assert that attributes send to the backend have no RumAttributes.ERROR_FINGERPRINT

0xnm · 2024-03-14T10:25:50Z

+            put(RumAttributes.ERROR_FINGERPRINT, fingerprint)
+        }
+        val throwableMessage = throwable.message
+        check(!throwableMessage.isNullOrBlank()) {


better to use the assumeTrue here, I don't think we want to fail the test if test setup is not good.

but anyway, is this needed here? what changes if message is null (it is never possible by looking on ThrowableForgeryFactory) or blank?

Since throwable.message is Stirng?, check performs the null check that allows us to no not perform the force dereference further down. I doesn't seem like assumeTrue does that.

yes, check is smart-cast friendly. however, we don't want test to fail if pre-condition is not satisfied, and assumeTrue will allow test to continue.

Speaking about this scenario, I think it is just easier to generate non-null message manually using @StringForgery fakeMessage: String, and then things will be simpler.

fuzzybinary · 2024-03-14T15:47:02Z

@0xnm / @mariusc83 Added stopResourceWithError fingerprinting. Can I get a re-review?

0xnm · 2024-03-14T16:33:32Z

        writer: DataWriter<Any>
    ) {
        attributes.putAll(GlobalRumMonitor.get(sdkCore).getAttributes())
+        val errorFingerprint = attributes.remove(RumAttributes.ERROR_FINGERPRINT) as? String


probably we need also to mention it in the related API of RumMonitor

fuzzybinary requested review from a team as code owners March 12, 2024 19:04

RUM-2907 Allow setting error fingerprint in log calls

c195c9a

This sets up an attribute, `LogAttributes.ERROR_FINGERPRINT` that is automatically transferred to the `Error` object in `LogEvent`s. Also fixes RUM-2209 by adding the `LogEvent.Error.fingerprint` member

fuzzybinary force-pushed the jward/RUM-2907-error-fingerprint branch from 1e3de3a to c195c9a Compare March 12, 2024 21:15

0xnm reviewed Mar 13, 2024

View reviewed changes

Comment thread ...roid-logs/src/test/kotlin/com/datadog/android/log/internal/domain/DatadogLogGeneratorTest.kt

mariusc83 approved these changes Mar 13, 2024

View reviewed changes

Comment thread reliability/single-fit/logs/src/test/kotlin/com/datadog/android/logs/integration/LoggerTest.kt

Add RUM error fingerprinting

6a0c439

fuzzybinary changed the title ~~RUM-2907 Allow setting error fingerprint in log calls~~ RUM-2907 Allow setting custom error fingerprint Mar 13, 2024

fuzzybinary requested a review from 0xnm March 13, 2024 15:18

0xnm reviewed Mar 13, 2024

View reviewed changes

Modifications from code review

9f07ba2

jonathanmos approved these changes Mar 14, 2024

View reviewed changes

0xnm approved these changes Mar 14, 2024

View reviewed changes

fuzzybinary requested review from 0xnm and mariusc83 March 14, 2024 15:46

0xnm approved these changes Mar 14, 2024

View reviewed changes

Add error fingerprinting to stopResourceWithError

68b3fc8

fuzzybinary force-pushed the jward/RUM-2907-error-fingerprint branch from 9c2fb7d to 68b3fc8 Compare March 14, 2024 18:51

fuzzybinary merged commit 30a864b into develop Mar 14, 2024

fuzzybinary deleted the jward/RUM-2907-error-fingerprint branch March 14, 2024 20:47

xgouchet added this to the 2.7.0 milestone Apr 5, 2024

	* Specifies a custom error fingerprint for the supplied.
	* Specifies a custom error fingerprint for the supplied error.

Uh oh!

Conversation

fuzzybinary commented Mar 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this PR do?

Review checklist (to be filled by reviewers)

Uh oh!

codecov-commenter commented Mar 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

0xnm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

fuzzybinary commented Mar 13, 2024

Uh oh!

0xnm commented Mar 13, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fuzzybinary commented Mar 14, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

fuzzybinary commented Mar 12, 2024 •

edited

Loading

codecov-commenter commented Mar 12, 2024 •

edited

Loading