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

feat(logs): Add error fingerprint attribute #1722

Merged
merged 5 commits into from
Mar 15, 2024
Merged

feat(logs): Add error fingerprint attribute #1722

merged 5 commits into from
Mar 15, 2024

Conversation

fuzzybinary
Copy link
Member

@fuzzybinary fuzzybinary commented Mar 12, 2024 •
edited
Loading

What and why?

Allow users to provide custom error fingerprint attribute.

In Logs the attribute Logs.Attributes.errorFingerprint is added to any log call and added to the LogEvent.Error.fingerprint member.

In RUM, the attribute RUM.Attributes.errorFingterprint is added to addError calls and it is transferred to the RUMError.finterprint member

refs: RUM-2906

How?

A special attribute (defined at Logs.Attributes.errorFingerprint) is read and moved during LogEvent creation

Review checklist

  • Feature or bugfix MUST have appropriate tests (unit, integration)
  • Make sure each commit and the PR mention the Issue number or JIRA reference
  • Add CHANGELOG entry for user facing changes

Custom CI job configuration (optional)

  • Run unit tests for Core, RUM, Trace, Logs, CR and WVT
  • Run unit tests for Session Replay
  • Run integration tests
  • Run smoke tests
  • Run tests for tools/

@fuzzybinary
feat(logs): Add error fingerprint attribute
58ed3aa 
Allow users to provide custom fingerprint in log calls that is transferred to the LogEvent

refs: RUM-2906
@fuzzybinary fuzzybinary force-pushed the jward/RUM-2906-error-fingerprint branch from 69127be to 58ed3aa Compare March 12, 2024 21:20
@datadog-datadog-prod-us1
Copy link

datadog-datadog-prod-us1 bot commented Mar 12, 2024 •
edited
Loading

Datadog Report

Branch report: jward/RUM-2906-error-fingerprint
Commit report: f1cdfbf
Test service: dd-sdk-ios

❌ 1 Failed (0 Known Flaky), 1238 Passed, 0 Skipped, 1m 26.81s Wall Time
🔻 Test Sessions change in coverage: 1 decreased (-0.05%), 3 increased, 2 no change

❌ Failed Tests (1)

  • testGivenStartedResource_whenResourceLoadingEndsWithErrorAndFingerprintAttribute_itSendsErrorEvent - RUMResourceScopeTests - Details

    Expand for error 
     Assertion Failure at RUMResourceScopeTests.swift:635: XCTAssertEqual failed: ("Optional(["_dd.error.fingerprint": "custom-fingerprint", "foo": "bar"])") is not equal to ("Optional(["foo": "bar"])")

🔻 Code Coverage Decreases vs Default Branch (1)

  • test DatadogTraceTests iOS 49.83% (-0.05%) - Details

@fuzzybinary fuzzybinary force-pushed the jward/RUM-2906-error-fingerprint branch from 6ab2625 to 90e91a3 Compare March 13, 2024 20:54
@fuzzybinary fuzzybinary marked this pull request as ready for review March 13, 2024 20:54
@fuzzybinary fuzzybinary requested review from a team as code owners March 13, 2024 20:54
ncreated
ncreated requested changes Mar 14, 2024
View reviewed changes
Copy link
Member

@ncreated ncreated left a comment

Choose a reason for hiding this comment

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

🎯 Well done 👍. There are some blocking points in public API comments.

Except in-line comments, what's our take on allowing fingerprint in RUM resource errors?

DatadogLogs/Sources/Feature/MessageReceivers.swift
@@ -68,6 +68,7 @@ internal struct LogMessageReceiver: FeatureMessageReceiver {
level: log.level,
message: log.message,
error: log.error,
errorFingerprint: nil,
Copy link
Member

Choose a reason for hiding this comment

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

suggestion/ (separate effort to consider rather than a change in this PR) This flow corresponds to sending logs from spans (span.log(fields:) API), so in theory it could support Trace.Attributes.errorFingerprint as well for consistency 💡.

Copy link
Member Author

Choose a reason for hiding this comment

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

As discussed at standup, it sounds like we won't support fingerprinting through traces at the moment.

DatadogLogs/Sources/Logs.swift Outdated Show resolved Hide resolved
DatadogLogs/Sources/RemoteLogger.swift
@@ -110,6 +110,7 @@ internal final class RemoteLogger: LoggerProtocol {
let tags = self.tags
var logAttributes = attributes
let isCrash = logAttributes?.removeValue(forKey: CrossPlatformAttributes.errorLogIsCrash) as? Bool ?? false
let errorFingerprint = logAttributes?.removeValue(forKey: Logs.Attributes.errorFingerprint) as? String
Copy link
Member

Choose a reason for hiding this comment

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

question/ Shouldn't we check if level is >= error 🤔, so this is not applied to lower severities?

Copy link
Member Author

Choose a reason for hiding this comment

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

No I don't think so, as you can pass errors to any level. I could check that an error was passed in though?

Copy link
Member

Choose a reason for hiding this comment

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

Ah, indeed 👍. All good 👌

DatadogRUM/Sources/RUM.swift Outdated Show resolved Hide resolved
IntegrationTests/Runner/Scenarios/RUM/ManualInstrumentation/SendRUMFixture3ViewController.swift Outdated Show resolved Hide resolved
@fuzzybinary @ncreated
Apply suggestions from code review
527715f 
Co-authored-by: Maciek Grzybowski <maciek.grzybowski@datadoghq.com>
@fuzzybinary fuzzybinary requested a review from ncreated March 14, 2024 15:19
@fuzzybinary fuzzybinary force-pushed the jward/RUM-2906-error-fingerprint branch from f1cdfbf to 8c6eb1b Compare March 14, 2024 16:27
ncreated
ncreated approved these changes Mar 15, 2024
View reviewed changes
Copy link
Member

@ncreated ncreated left a comment

Choose a reason for hiding this comment

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

Looks good 👍

PS. I removed IPHONEOS_DEPLOYMENT_TARGET overrides in 45229f7 as they went again into the change 😉

@fuzzybinary fuzzybinary merged commit 77c1f2a into develop Mar 15, 2024
18 checks passed
This was referenced Mar 19, 2024
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ncreated ncreated ncreated approved these changes

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

Successfully merging this pull request may close these issues.
2 participants
@fuzzybinary @ncreated