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

Allow manually tracked resources to detect first party hosts #837

Merged
merged 5 commits into from
May 6, 2022

Conversation

fuzzybinary
Copy link
Member

@fuzzybinary fuzzybinary commented May 2, 2022

What and why?

First party hosts set by trackUrlSession only works for automatically instrumented URLSessions. This allows manual calls to startResourceLoading and stopResourceLoading to detect first party host calls as well.

How?

Not sure if this is the approach we want to take, but it made sense to me. I'm pulling the configuration onto the RUM feature configuration, then using that in the dependency configuration for the application scope to create the same Filter object that the URLSessionDelegate uses.

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
  • Run integration tests
  • Run smoke tests

@fuzzybinary fuzzybinary requested a review from a team as a code owner May 2, 2022 18:40
@fuzzybinary fuzzybinary force-pushed the jward/RUMM-2157-first-party-hosts branch from 976d24d to afc8c1c Compare May 2, 2022 19:07
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.

I like the idea of adding url filter to RUMScopeDependencies, with that I don't think we need the isFirstPartyRequest property in RUMStartResourceCommand anymore. We can simply use the filter while processing the command, WDYT?

@fuzzybinary
Copy link
Member Author

Yeah that would simplify things. I'll give it a shot.

@fuzzybinary
Copy link
Member Author

@maxep Updated, let me know if this is what you meant.

@maxep
Copy link
Member

maxep commented May 4, 2022

Yes, exactly! I left another /nit

Also, could you rebase on develop? I've merge the release branch since :)

First party hosts set by `trackUrlSession` only works for automatically instrumented URLSessions. This allows manual calls to startResourceLoading and stopResourceLoading to detect first party host calls as well.
Instead, use the firstPartyURLsFilter on the dependencies to set isFirstPartyResource as we're writing the resource.
@fuzzybinary fuzzybinary force-pushed the jward/RUMM-2157-first-party-hosts branch from 62c429d to e3dad80 Compare May 4, 2022 17:32
Checking first party resource a the last possible minute, avoiding passing it through multiple scopes.
@fuzzybinary fuzzybinary requested a review from maxep May 5, 2022 20:46
@fuzzybinary
Copy link
Member Author

@maxep Moved it up one more level.

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.

Thanks 👌

Tests/DatadogTests/Datadog/Mocks/RUMFeatureMocks.swift Outdated Show resolved Hide resolved
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.

✨ 👌

Co-authored-by: Maxime Epain <maxime.epain@datadoghq.com>
@fuzzybinary fuzzybinary merged commit aefc32c into develop May 6, 2022
@ncreated ncreated mentioned this pull request Jul 21, 2022
6 tasks
@fuzzybinary fuzzybinary deleted the jward/RUMM-2157-first-party-hosts branch March 13, 2023 17: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.

3 participants