Skip to content

RUM-9747 Strongly-typed RUM Additional Context #2290

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
Merged
merged 3 commits into from
May 14, 2025
Merged

RUM-9747 Strongly-typed RUM Additional Context #2290

merged 3 commits into from
May 14, 2025

Conversation

maxep
Copy link
Member

@maxep maxep commented May 7, 2025 •
edited
Loading

What and why?

Following: #2289

FeatureBaggage comes with perf implications, we are replacing loosely-typed baggages in core context by strongly-typed additional context.

How?

Replace RUM baggage by RUMCoreContext as additional context.

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
  • Add Objective-C interface for public APIs (see our guidelines (internal) and run make api-surface)

@maxep maxep requested review from a team as code owners May 7, 2025 17:01
@maxep maxep force-pushed the maxep/RUM-9747/rum-addtional-context branch from f9eb92a to 26c9219 Compare May 7, 2025 17:01
@datadog-datadog-prod-us1
Copy link

datadog-datadog-prod-us1 bot commented May 7, 2025 •
edited
Loading

Datadog Report

Branch report: maxep/RUM-9747/rum-addtional-context
Commit report: 9a71de0
Test service: dd-sdk-ios

✅ 0 Failed, 2397 Passed, 1495 Skipped, 2m 49.52s Total duration (1m 56.05s time saved)

@maxep maxep force-pushed the maxep/RUM-9747/rum-addtional-context branch from 26c9219 to 43b7028 Compare May 9, 2025 08:17
@maxep maxep force-pushed the maxep/RUM-9747/migrate-baggage-to-additional-context branch from cabcd8e to 77208b0 Compare May 9, 2025 10:34
@maxep maxep force-pushed the maxep/RUM-9747/rum-addtional-context branch 3 times, most recently from 64365bd to 9a71de0 Compare May 9, 2025 13:52
Base automatically changed from maxep/RUM-9747/migrate-baggage-to-additional-context to develop May 12, 2025 12:31
@maxep maxep force-pushed the maxep/RUM-9747/rum-addtional-context branch from 9a71de0 to eb6381b Compare May 12, 2025 14:23
@datadog-datadog-prod-us1
Copy link

datadog-datadog-prod-us1 bot commented May 12, 2025 •
edited
Loading

Datadog Summary

✅ Code Quality    ✅ Code Security    ✅ Dependencies

Test Optimization Report

Branch report: maxep/RUM-9747/rum-addtional-context
Commit report: eb6381b
Test service: dd-sdk-ios

✅ 0 Failed, 2467 Passed, 1421 Skipped, 2m 50.7s Total Time

Was this helpful? Give us feedback!

ncreated
ncreated previously approved these changes May 13, 2025
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.

Overall, it looks good 👍 I left a few questions about the internal API and challenged a couple of design choices 🙂.

DatadogInternal/Sources/Context/DatadogContext.swift Outdated
Comment on lines 187 to 199
/// Gets an additional context value.
///
/// - Parameter type: The value type.
/// - Parameter type: The additional context type.
/// - Returns: The `Context` if found
public func additionalContext<Context>(ofType type: Context.Type = Context.self) -> Context? where Context: AdditionalContext {
additionalContext[type.key] as? Context
}

/// Gets an additional context value.
/// - Parameter context: The additional context.
public mutating func set<Context>(additionalContext context: Context?) where Context: AdditionalContext {
additionalContext[Context.key] = context
}
Copy link
Member

Choose a reason for hiding this comment

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

api/ The current API comments are too vague and don't provide any meaningful information beyond what the method names already imply. In particular, the comment for set(additionalContext:) appears to be incorrect—likely copy-pasted from elsewhere.

Since API comments are currently our primary form of architectural documentation (unfortunately), they warrant careful attention. As a reader, I’d expect the comments to explain key concepts—like what "additional context" actually represents, how to add / update or remove one. Without that, these APIs come across as opaque and disconnected.

To avoid redundancy, this explanation can be centralized in the documentation for the AdditionalContext type itself, and symbol links can be used to reference it from related APIs.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes indeed, I will add more details there 👍

Copy link
Member Author

Choose a reason for hiding this comment

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

I have added more doc with link to core's method to mutate the context, LMKWYT!

DatadogInternal/Sources/Context/DatadogContext.swift

/// Gets an additional context value.
/// - Parameter context: The additional context.
public mutating func set<Context>(additionalContext context: Context?) where Context: AdditionalContext {
Copy link
Member

Choose a reason for hiding this comment

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

question/ How can additionalContext be removed after it was previously set? 🤔 💭

Copy link
Member Author

Choose a reason for hiding this comment

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

By using this method, I can also add a method to the context itself 👍

@maxep maxep requested a review from ncreated May 13, 2025 14:30
ncreated
ncreated approved these changes May 13, 2025
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.

🎯

@ncreated ncreated mentioned this pull request May 14, 2025
Closed
@maxep
Copy link
Member Author

maxep commented May 14, 2025

/merge

@dd-devflow
Copy link

dd-devflow bot commented May 14, 2025 •
edited
Loading

View all feedbacks in Devflow UI.

2025-05-14 07:44:37 UTC ℹ️ Start processing command /merge

2025-05-14 07:44:43 UTC ℹ️ MergeQueue: pull request added to the queue

The expected merge time in develop is approximately 1h (p90).

2025-05-14 08:04:05 UTC ❌ MergeQueue: The checks failed on this merge request

Tests failed on this commit cd25762:

What to do next?

  • Investigate the failures and when ready, re-add your pull request to the queue!
  • If your PR checks are green, try to rebase/merge. It might be because the CI run is a bit old.
  • Any question, go check the FAQ.

@maxep
Copy link
Member Author

maxep commented May 14, 2025

/merge

@dd-devflow
Copy link

dd-devflow bot commented May 14, 2025 •
edited
Loading

View all feedbacks in Devflow UI.

2025-05-14 08:13:22 UTC ℹ️ Start processing command /merge

2025-05-14 08:13:27 UTC ℹ️ MergeQueue: pull request added to the queue

The expected merge time in develop is approximately 1h (p90).

2025-05-14 08:56:04 UTC ℹ️ MergeQueue: This merge request was merged

@dd-mergequeue dd-mergequeue bot merged commit 48dcc86 into develop May 14, 2025
15 checks passed
@dd-mergequeue dd-mergequeue bot deleted the maxep/RUM-9747/rum-addtional-context branch May 14, 2025 08:56
@maxep maxep mentioned this pull request May 20, 2025
Merged
4 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
mergequeue-status: done
Projects
None yet
Milestone
No milestone
Development

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