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

RUM-1833 feat: Link (emergency) crash Logs to crashed RUM session #1645

Merged
merged 3 commits into from
Jan 25, 2024

Conversation

ncreated
Copy link
Member

@ncreated ncreated commented Jan 24, 2024

What and why?

📦 This PR adds link to the RUM session for crash reports sent as emergency Log event.

See:

How?

With the "last RUMViewEvent" being already encoded in CrashContext.data, this PR adds extra logic in DatadogLogs to decode RUM session attributes. Decoding is limited only to these fields that are required so we don't decode the whole event (that wouldn't be even possible as DatadogLogs has no dependency on DatadogRUM).

Once decoded, these attributes are attached to LogEvent following the contract between SDK and Datadog app.

Except unit tests covering edge cases (like malformed RUM view information), this PR also adds integration unit tests for "Sending Crash Reports as Logs and RUM".

Last, I took extra time to document logs, spans and RUM correlations in this spec (internal).

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/

@ncreated ncreated self-assigned this Jan 24, 2024
Comment on lines -174 to +195
.error("Fails to decode crash from RUM", error: error)
.error("Failed to decode crash message in `LogMessageReceiver`", error: error)
Copy link
Member Author

Choose a reason for hiding this comment

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

This is to differentiate it from other telemetry with the same message.

@ncreated ncreated marked this pull request as ready for review January 24, 2024 15:14
@ncreated ncreated requested review from a team as code owners January 24, 2024 15:14
Comment on lines +158 to +172
struct PartialRUMViewEvent: Decodable {
struct Application: Decodable {
let id: String
}
struct Session: Decodable {
let id: String
}
struct View: Decodable {
let id: String
}

let application: Application
let session: Session
let view: View
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any public/internal doc explaining how this correlation work? In case when there an update needed we can always refer.

Also, we have been doing quite a few changes to make the correlation work and so far they have been very organic. These things impact SDK and we should document them somewhere.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fair call 👍. I took an extra time to document logs, spans and RUM correlations in this spec (internal).

Copy link
Contributor

@ganeshnj ganeshnj 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 overall coverage.

A general comment on improving the documentation. Not blocking.

@ncreated ncreated merged commit e01e97b into develop Jan 25, 2024
10 checks passed
@ncreated ncreated deleted the ncreated/RUM-1833/link-crash-logs-to-rum-session branch January 25, 2024 11:44
@ncreated ncreated mentioned this pull request Jan 25, 2024
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants