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

RUM-4311: Add Logs event mapper to ObjC API #2008

Merged
merged 1 commit into from
Aug 22, 2024
Merged

RUM-4311: Add Logs event mapper to ObjC API #2008

merged 1 commit into from
Aug 22, 2024

Conversation

0xnm
Copy link
Member

@0xnm 0xnm commented Aug 21, 2024

What and why?

This PR adds setEventMapper ObjC API for logs.

How?

A brief description of implementation details of this PR.

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

@0xnm 0xnm force-pushed the nogorodnikov/rum-4311/add-logs-event-mapper-objc-api branch from e63ed62 to f687c3c Compare August 21, 2024 14:07
@0xnm 0xnm marked this pull request as ready for review August 21, 2024 14:10
@0xnm 0xnm requested review from a team as code owners August 21, 2024 14:10
@datadog-datadog-prod-us1
Copy link

datadog-datadog-prod-us1 bot commented Aug 21, 2024 •
edited
Loading

Datadog Report

Branch report: nogorodnikov/rum-4311/add-logs-event-mapper-objc-api
Commit report: a5bf5c0
Test service: dd-sdk-ios

✅ 0 Failed, 1864 Passed, 0 Skipped, 1m 16.01s Total Time
🔻 Test Sessions change in coverage: 4 decreased, 4 no change

🔻 Code Coverage Decreases vs Default Branch (4)

  • test DatadogCoreTests iOS 73.16% (-0.29%) - Details
  • test DatadogSessionReplayTests iOS 30.48% (-0.2%) - Details
  • test DatadogRUMTests iOS 81.74% (-0.04%) - Details
  • test DatadogTraceTests iOS 54.28% (-0.03%) - Details

@0xnm 0xnm force-pushed the nogorodnikov/rum-4311/add-logs-event-mapper-objc-api branch from f687c3c to a5bf5c0 Compare August 21, 2024 14:19
ncreated
ncreated reviewed Aug 22, 2024
View reviewed changes
DatadogObjc/Sources/Logs/LogsDataModels+objc.swift
Copy link
Member

Choose a reason for hiding this comment

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

This looks 💯. How was it created? Long-term we may want to generate both Swift and Objc models through rum-models-generator CLI - I wonder how "far" is this file from the eventual result the generator may give us. It looks pretty solid and follows all the conventions from RUM models.

Copy link
Member Author

Choose a reason for hiding this comment

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

It was created manually. I just took RUM models for the inspiration.

Copy link
Member

Choose a reason for hiding this comment

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

Got it 👍. Sounds good as it solves current problem 👌. I hope we find time to code-gen this from JSON schema in 3.x.

ncreated
ncreated approved these changes Aug 22, 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 👌, great work on +objc models 👍.

DatadogObjc/Sources/Logs/LogsDataModels+objc.swift
Copy link
Member

Choose a reason for hiding this comment

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

Got it 👍. Sounds good as it solves current problem 👌. I hope we find time to code-gen this from JSON schema in 3.x.

@0xnm 0xnm merged commit 691fc07 into develop Aug 22, 2024
16 checks passed
@mariedm mariedm mentioned this pull request Sep 11, 2024
Merged
3 tasks
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
@0xnm @ncreated