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

Integration with CI Visibility tests #761

Merged
merged 7 commits into from
Mar 17, 2022
Merged

Conversation

nachoBonafonte
Copy link
Contributor

What and why?

This PR enables a much deeper integration of RUM with CI Visibility product (dd-sdk-swift-testing). If a UI test instrumented with CI Visibility is run and a RUM sessions is configured, the session will be identified as a created by CI App and will be shown as part of the test.

There is a related PR with the needed changes in the dd-sdk-swift-testing framework

How?

RUM now detects on loading if is running under an instrumented test by checking if the environment variable CI_VISIBILITY_TEST_EXECUTION_ID is available, which contains the test identifier. If so it makes the following changes:

  • Sets session.type to ci-test
  • Injects ci_visibility.test_execution_id to every event
  • Set _dd.origin to ciapp-test so that APM traces from RUM in CI Visibility mode can be distinguished.
  • Send a message to a Mach Port initialized in the CI Visibility framework (TestMachPort) to notify RUM is enabled
  • Creates a new Mach Port (RUMMachPort) that will receive a message when the test ends. RUM will flush the RUM events when the message is received.

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 change.

Ignacio Bonafonte added 2 commits February 22, 2022 09:08
RUM is now able to detect if its being run inside a test instrumented with CI Visibility and perform the following changes then:
* Sets `session.type` to `ci_test`
* Injects `ci_visibility.test_execution_id` to every event
* Set `_dd.origin` to `ciapp-test` so that APM traces from RUM in CI Visibility mode can be distinguished.
* Send a message to a Mach Port initialized in the CI Visibility framework (TestMachPort) to notify RUM is enabled
* Creates a new Mach Port (RUMMachPort) that will receive a message when the test ends. RUM will flush the RUM events when the message is received.
…` value when setting instead of in initialisation.

Fix some tests
@maxep
Copy link
Member

maxep commented Feb 23, 2022

Very nice use of CFMessagePortSendRequest, and I like that it is implemented as a FeatureIntegration.
Tho, I'm a bit concern of making it a transverse dependency by using CITestIntegration.ciTestExecutionID, it will surely conflict with our effort in v2. Could it be injected somehow? by making it part of RUMScopeDependencies for instance 🤔

Also, I'm fine with exposing flushing methods internally 👍

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 really good 👍. I left few suggestions on making this integration fit our codebase a bit simpler.

Ignacio Bonafonte added 4 commits February 23, 2022 16:15
- Modify CITestIntegration to be object oriented
- Change a messageID value to be a bit more descriptive
- Fix some tests
Add a test to validate CITestIntegration is off by default
@nachoBonafonte nachoBonafonte marked this pull request as ready for review March 8, 2022 10:11
@nachoBonafonte nachoBonafonte requested review from a team as code owners March 8, 2022 10:11

**Note**: Tracing auto instrumentation uses `URLSession` swizzling, but it is opt-in: if you do not specify `firstPartyHosts`, no swizzling is applied.
**Note**: Tracing auto-instrumentation uses `URLSession` swizzling and is opt-in. If you do not specify `firstPartyHosts`, swizzling is not applied.
Copy link
Contributor

Choose a reason for hiding this comment

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

Is "swizzling" defined elsewhere, or is it a commonly understood term? It is a bit confusing to me, but this is outside my normal subject area.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, is a commonly understood term in iOS programming. Anyway this change was not supposed to be here so I reverted it

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.

👍

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.

👌👏

@nachoBonafonte nachoBonafonte merged commit 7815a47 into develop Mar 17, 2022
@nachoBonafonte nachoBonafonte deleted the CI-APP-RUM-Integration branch March 17, 2022 08:48
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