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

feat: Provide a method for late initialization of URLSessionTracking #1593

Merged

Conversation

fuzzybinary
Copy link
Member

What and why?

This is for a limited use case of people that need to initialize native URL session tracking after RUM has been initialized.

refs: RUM-2525

How?

The approach here is a bit weird because of the need to include some providers (the Trace ID generator and the date provider) in the configuration. We really only override these in unit tests, and they're internal, but I needed to provide a way to include the overrides, hence the new Configuration.LateURLSessionTracking object.

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 fuzzybinary requested review from a team as code owners December 15, 2023 22:07
This is for a limited use case of people that need to initialize native URL session tracking after RUM has been initialized.

refs: RUM-2525
@fuzzybinary fuzzybinary force-pushed the jward/RUM-2525-late-url-session-instrumentation branch from ebc30f1 to daf7ca5 Compare December 18, 2023 15:51
@datadog-datadog-prod-us1
Copy link

datadog-datadog-prod-us1 bot commented Dec 18, 2023

Datadog Report

Branch report: jward/RUM-2525-late-url-session-instrumentation
Commit report: daf7ca5

dd-sdk-ios: 0 Failed, 0 New Flaky, 2507 Passed, 0 Skipped, 21m 18.82s Wall Time

@ganeshnj
Copy link
Contributor

A bit lost with the requirement of this change. Do you have a code example where the current version doesn't work.

In current available version, either RUM or Tracing must be enabled to enable URLSession tracking.

DatadogRUM/Sources/RUM+Internal.swift Outdated Show resolved Hide resolved
DatadogRUM/Sources/RUM+Internal.swift Outdated Show resolved Hide resolved
DatadogRUM/Sources/RUM+Internal.swift Outdated Show resolved Hide resolved
DatadogRUM/Tests/RUMTests.swift Outdated Show resolved Hide resolved
@fuzzybinary
Copy link
Member Author

@ganeshnj This is a very strange use case of a Flutter user that uses native iOS URLSessions. Flutter initializes Datadog and RUM, but they need a way to enable URLSession tracking after initialization has been performed.

@fuzzybinary fuzzybinary requested a review from maxep December 20, 2023 14:57
@ganeshnj
Copy link
Contributor

@ganeshnj This is a very strange use case of a Flutter user that uses native iOS URLSessions. Flutter initializes Datadog and RUM, but they need a way to enable URLSession tracking after initialization has been performed.

I see, I wonder if we could use the existing config type with default values and extend it with implementations that you want to inject, because this type will go to public type (in auto completions et all) and makes the API unintuitive.

@fuzzybinary
Copy link
Member Author

@ganeshnj I'm not sure how to do this cleanly without a new type or by adding more parameters to enableURLSessionTracking. Do you have an example of how you'd structure it?

Alternatively, we could put LateURLSessionTracking under another wrapper Internal or __Internal to avoid it showing up at the top level of autocomplete?

maxep
maxep previously approved these changes Dec 20, 2023
Copy link
Member

@maxep maxep left a comment

Choose a reason for hiding this comment

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

Nice, thank you for the changes and the fuzzy tests 🙏

Comment on lines 72 to 73
/// - Parameters:
/// - in: the configuration for URL session tracking
Copy link
Member

Choose a reason for hiding this comment

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

This list of parameters seems wrong 🖐️

Comment on lines 74 to 77
public static func enableURLSessionTracking(
with configuration: RUM.Configuration.LateURLSessionTracking,
in core: DatadogCoreProtocol = CoreRegistry.default) throws {
Copy link
Member

Choose a reason for hiding this comment

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

question/ Have we considered changing this to following?

    public static func enableURLSessionTracking(
        with urlSessionTrackingConfiguration: RUM.Configuration.URLSessionTracking,
        rumConfiguration: RUM.Configuration,
        in core: DatadogCoreProtocol = CoreRegistry.default) throws {

Effectively, this way we won't need to introduce new LateURLSessionTracking nor duplicate any properties. The trade-off will be on the caller-side (in Flutter), where we'll need to supply this method both configurations:

  • RUM.Configuration.URLSessionTracking
  • RUM.Configuration

For the sake of streamlining RUM init, the cost seems worth it. WDYT?

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'm concerned with requiring RUM.Configuration because of the number of parameters available that just aren't used, especially the non-optional parameters that wouldn't actually have an affect.

Remember "on the Flutter side" in this case is a client who has to make sense of the API, and requiring the RUM.Configuration when only a few parameters out of it are going to be used worries me. I'm open to discuss it though.

Copy link
Member

Choose a reason for hiding this comment

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

I see 👍. One more idea that comes to my mind is attaching RUM.Configuration to RUMFeature, so we can retrieve it "after" by ourselves without requiring the client to pass it:

public static func enableURLSessionTracking(
   with urlSessionTrackingConfiguration: RUM.Configuration.URLSessionTracking,
   in core: DatadogCoreProtocol = CoreRegistry.default
) throws {
   guard let rum = core.get(feature: RUMFeature.self) else {
      throw ProgrammerError(
         description: "RUM must be initialized before calling `RUM.enableUrlSessionTracking`."
      )
   }
   let rumConfiguration = rum.configuration // <-- ⭐️ new thing
   // ...

This isn't a change request. Using separate LateURLSessionTracking type makes sense as well 👍.

@fuzzybinary fuzzybinary force-pushed the jward/RUM-2525-late-url-session-instrumentation branch from 8d142dd to 53cb5ec Compare January 2, 2024 14:30
@fuzzybinary fuzzybinary requested a review from ncreated January 2, 2024 14:31
@datadog-datadog-prod-us1
Copy link

datadog-datadog-prod-us1 bot commented Jan 2, 2024

Datadog Report

Branch report: jward/RUM-2525-late-url-session-instrumentation
Commit report: 7e58f00

dd-sdk-ios: 0 Failed, 0 New Flaky, 2529 Passed, 0 Skipped, 20m 30.17s Wall Time

ncreated
ncreated previously approved these changes Jan 2, 2024
Comment on lines 74 to 77
public static func enableURLSessionTracking(
with configuration: RUM.Configuration.LateURLSessionTracking,
in core: DatadogCoreProtocol = CoreRegistry.default) throws {
Copy link
Member

Choose a reason for hiding this comment

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

I see 👍. One more idea that comes to my mind is attaching RUM.Configuration to RUMFeature, so we can retrieve it "after" by ourselves without requiring the client to pass it:

public static func enableURLSessionTracking(
   with urlSessionTrackingConfiguration: RUM.Configuration.URLSessionTracking,
   in core: DatadogCoreProtocol = CoreRegistry.default
) throws {
   guard let rum = core.get(feature: RUMFeature.self) else {
      throw ProgrammerError(
         description: "RUM must be initialized before calling `RUM.enableUrlSessionTracking`."
      )
   }
   let rumConfiguration = rum.configuration // <-- ⭐️ new thing
   // ...

This isn't a change request. Using separate LateURLSessionTracking type makes sense as well 👍.

@fuzzybinary
Copy link
Member Author

I would actually prefer adding the configuration to the feature if we're good with that.

@fuzzybinary
Copy link
Member Author

Changed to saving the configuration. It's much cleaner, even if it does mean holding onto some extra info.

ncreated
ncreated previously approved these changes Jan 3, 2024
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.

LGTM 👍. Thanks for extra efforts on simplification 🙂

@fuzzybinary fuzzybinary force-pushed the jward/RUM-2525-late-url-session-instrumentation branch from 3d06a06 to 5b67f27 Compare January 3, 2024 13:07
@fuzzybinary fuzzybinary requested a review from ncreated January 3, 2024 14:14
@fuzzybinary fuzzybinary merged commit 5073cf6 into develop Jan 4, 2024
19 checks passed
@fuzzybinary fuzzybinary deleted the jward/RUM-2525-late-url-session-instrumentation branch January 4, 2024 14:53
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.

4 participants