Skip to content

[RUM-9617] - Base Benchmark app with support for Test Scenarios and external config#881

Merged
sbarrio merged 1 commit into
sbarrio/RUM-9457/automated-regression-detecting-benchmark-systemfrom
sbarrio/RUM-9617/base-benchmark-test-app-with-scenario-support
Jun 2, 2025
Merged

[RUM-9617] - Base Benchmark app with support for Test Scenarios and external config#881
sbarrio merged 1 commit into
sbarrio/RUM-9457/automated-regression-detecting-benchmark-systemfrom
sbarrio/RUM-9617/base-benchmark-test-app-with-scenario-support

Conversation

@sbarrio
Copy link
Copy Markdown
Contributor

@sbarrio sbarrio commented May 19, 2025

What does this PR do?

This PR implements a base implementation for an app that can be internally used as a benchmark tester of different scenarios. It lays out the groundwork needed to be able to set up different kinds of builds, tests and related configurations so test scenarios can be run under different environments and instrumentation conditions. It allows for new scenarios to be added easily, allowing them to be instrumented with the Datadog SDK. It also enables a set of vitals tracking systems at the native level so metrics on CPU/Memory/FPS are tracked and reported too.

Motivation

We want to have a set of automatic benchmark tests run every time we merge code to develop.

benchmarks

Additional Notes

This PR on itself does not yet implement any real test scenario nor sets up a CI pipeline for tests. That will come on the next PRs right after this one. This is just laying the groundwork needed for those to work.

Review checklist (to be filled by reviewers)

  • Feature or bugfix MUST have appropriate tests
  • Make sure you discussed the feature or bugfix with the maintaining team in an Issue
  • Make sure each commit and the PR mention the Issue number (cf the CONTRIBUTING doc)
  • If this PR is auto-generated, please make sure also to manually update the code related to the change

@sbarrio sbarrio force-pushed the sbarrio/RUM-9617/base-benchmark-test-app-with-scenario-support branch from 094683c to a348fb9 Compare May 20, 2025 08:01
@datadog-datadog-prod-us1
Copy link
Copy Markdown

datadog-datadog-prod-us1 Bot commented May 20, 2025

Datadog Report

Branch report: sbarrio/RUM-9617/base-benchmark-test-app-with-scenario-support
Commit report: a0f8388
Test service: dd-sdk-reactnative

✅ 0 Failed, 661 Passed, 1 Skipped, 3.76s Total Time

@sbarrio sbarrio force-pushed the sbarrio/RUM-9617/base-benchmark-test-app-with-scenario-support branch from a41dfe2 to 5b8bda0 Compare May 26, 2025 07:58
implementation "com.datadoghq:dd-sdk-android-rum:2.21.0"
implementation "com.datadoghq:dd-sdk-android-logs:2.21.0"
implementation "com.datadoghq:dd-sdk-android-trace:2.21.0"
implementation "com.datadoghq:dd-sdk-android-webview:2.21.0"
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This PR needs Android SDK 2.21.0 as the Vitals tracking module was compiled with that version (it relies on Metrics improvements done on that version).

For this, we'll need to merge this PR first.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

We now include the benchmark tools directly from Maven, but I still want to keep them at 2.21.0 since there were important changes included on that version that our Vitals tracking native modules rely on.

Comment thread benchmarks/android/app/build.gradle Outdated
// From the release branch of dd-sdk-android, run:
// ./gradlew :tools:benchmark:assembleRelease
// Then copy the resulting .aar into benchmarks/android/app/libs
implementation(name: 'benchmark-release', ext: 'aar')
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The benchmark app relies on the tools/benchmarks vitals tracking code on dd-sdk-android, however, that code is not published as a standalone dependency that can be imported into different projects, so at least for now, I opted for including it as a precompiled .aar into this package. This means that we need to recompile it whenever we bump the Android version.

On next iterations we can replace this with an external dependency if we decide to make the benchmarks project public.

Copy link
Copy Markdown
Member

@marco-saia-datadog marco-saia-datadog May 28, 2025

Choose a reason for hiding this comment

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

I believe this and the iOS code duplication are the only critical issues to address, as manually assembling the AAR for each bump of the Android SDK version would be too complex, and maintaining many duplicate iOS classes is not ideal.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

you can already include the tools:benchmark module like this implementation "com.datadoghq:benchmark:2.19.2". It is published.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks @aleksandr-gringauz, let me give it a try. I assume 2.21.0 is also available?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It does indeed work. Thanks for raising this 🙇

@marco-saia-datadog I'll ask the iOS SDK team for support on this in case there's a similar solution possible on their end.

Comment thread benchmarks/.env.alternate
Comment thread benchmarks/ios/BenchmarkVitals/BenchmarkVitals.mm
import OpenTelemetryApi

internal final class Meter: DatadogInternal.BenchmarkMeter {
let meter: OpenTelemetryApi.Meter
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

These classes are a 1:1 match with the ones found on the iOS Benchmark app code.

Ideally, we should be able to only import this as an external dependency, but SPM does not allow for specific folder imports from Github, just whole projects.

@sbarrio sbarrio marked this pull request as ready for review May 26, 2025 08:35
@sbarrio sbarrio requested review from a team as code owners May 26, 2025 08:35
Comment thread benchmarks/src/testSetup/testUtils.ts Outdated
janine-c
janine-c previously approved these changes May 26, 2025
Comment thread benchmarks/README.md Outdated
Comment thread benchmarks/README.md Outdated
Comment thread benchmarks/README.md Outdated
Comment thread benchmarks/README.md Outdated
Comment thread benchmarks/README.md Outdated
Comment thread benchmarks/README.md Outdated
Comment thread benchmarks/README.md Outdated
Comment thread benchmarks/README.md Outdated
Comment thread benchmarks/README.md Outdated
Comment thread benchmarks/README.md Outdated
@sbarrio sbarrio self-assigned this May 27, 2025
@cdn34dd
Copy link
Copy Markdown
Contributor

cdn34dd commented May 27, 2025

If this PR does not yet implement any test scenarios, should this not be merged onto a feature brach, and only push to develop once the full behavior is implemented ?

Copy link
Copy Markdown
Member

@marco-saia-datadog marco-saia-datadog left a comment

Choose a reason for hiding this comment

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

@sbarrio Amazing work here! 🙌 🎉

As @cdn34dd suggested, let's change the target to a feature branch, and let's only merge it to develop when we have at least one real test scenario, and a CI job to run it.

@sbarrio
Copy link
Copy Markdown
Contributor Author

sbarrio commented May 28, 2025

If this PR does not yet implement any test scenarios, should this not be merged onto a feature brach, and only push to develop once the full behavior is implemented ?

@sbarrio Amazing work here! 🙌 🎉

As @cdn34dd suggested, let's change the target to a feature branch, and let's only merge it to develop when we have at least one real test scenario, and a CI job to run it.

Thanks for the reviews 🙇

I'll create a feature branch as suggested, so things are cleaner 👍

@sbarrio sbarrio dismissed stale reviews from marco-saia-datadog and janine-c via 1c4993c May 28, 2025 08:16
@sbarrio sbarrio force-pushed the sbarrio/RUM-9617/base-benchmark-test-app-with-scenario-support branch from 5b8bda0 to 1c4993c Compare May 28, 2025 08:16
@sbarrio sbarrio changed the base branch from develop to sbarrio/RUM-9457/automated-regression-detecting-benchmark-system May 28, 2025 08:16
Comment thread benchmarks/metro.config.js Outdated
Comment thread benchmarks/src/testSetup/testUtils.ts Outdated
Comment thread benchmarks/src/testSetup/testUtils.ts Outdated
Comment thread benchmarks/src/scenario/Default/defaultScenario.tsx Outdated
Comment thread benchmarks/src/scenario/NavigationExample/navigationExampleScenario.tsx Outdated
Comment thread benchmarks/src/App.tsx Outdated
@sbarrio sbarrio force-pushed the sbarrio/RUM-9617/base-benchmark-test-app-with-scenario-support branch from 1c4993c to 1f907df Compare May 28, 2025 08:31
@sbarrio
Copy link
Copy Markdown
Contributor Author

sbarrio commented May 28, 2025

@janine-c Thanks a ton for your thorough review. I have updated the README.md with your suggestions and have clarified some parts 🙇

@sbarrio sbarrio force-pushed the sbarrio/RUM-9617/base-benchmark-test-app-with-scenario-support branch from 1f907df to 8403f09 Compare May 28, 2025 08:45
@sbarrio sbarrio force-pushed the sbarrio/RUM-9617/base-benchmark-test-app-with-scenario-support branch from 8403f09 to ad1921f Compare May 28, 2025 09:29
@sbarrio sbarrio force-pushed the sbarrio/RUM-9617/base-benchmark-test-app-with-scenario-support branch from ad1921f to a0f8388 Compare May 28, 2025 10:17
Copy link
Copy Markdown
Contributor

@cdn34dd cdn34dd 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, well done.

Copy link
Copy Markdown

@janine-c janine-c left a comment

Choose a reason for hiding this comment

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

Looks great, thank you for applying those edits so quickly! 🚀

@sbarrio sbarrio merged commit 70bc798 into sbarrio/RUM-9457/automated-regression-detecting-benchmark-system Jun 2, 2025
6 checks passed
@sbarrio sbarrio deleted the sbarrio/RUM-9617/base-benchmark-test-app-with-scenario-support branch June 2, 2025 12: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.

5 participants