dd-sdk-android icon indicating copy to clipboard operation
dd-sdk-android copied to clipboard

RUM-5754: Add profiling interface to sdk core

Open ambushwork opened this issue 1 year ago • 3 comments

What does this PR do?

Adds following interface into SDK core:

  • BenchmarkCounter
  • BenchmarkGauge
  • BenchmarkProfiler
  • BenchmarkSpan
  • BenchmarkTracer

Adds an object class to for BenchmarkProfiler

  • GlobalBenchmark

Motivation

RUM-5754

Review checklist (to be filled by reviewers)

  • [ ] Feature or bugfix MUST have appropriate tests (unit, integration, e2e)
  • [ ] 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)

ambushwork avatar Aug 14 '24 12:08 ambushwork

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 69.95%. Comparing base (a99fef0) to head (7d1d14c). Report is 6 commits behind head on feature/profiling.

Additional details and impacted files
@@                  Coverage Diff                  @@
##           feature/profiling    #2194      +/-   ##
=====================================================
+ Coverage              69.94%   69.95%   +0.02%     
=====================================================
  Files                    726      726              
  Lines                  26989    27006      +17     
  Branches                4524     4528       +4     
=====================================================
+ Hits                   18876    18892      +16     
+ Misses                  6841     6837       -4     
- Partials                1272     1277       +5     

see 36 files with indirect coverage changes

codecov-commenter avatar Aug 14 '24 13:08 codecov-commenter

I'm not sure about the approach we are taking here: why this is added to the SDK core module? Where implementations are supposed to be?

The main issue is that SDK core is what is distributed to the clients, while we are working just on the internal tooling to benchmark the SDK (it is not supposed to be published). So we are adding something for the internal needs and this is being added to the binary used by the clients - they don't need it.

Also once it is published, it may give a wrong impression on the client side that we support metrics in the SDK, which is not true.

0xnm avatar Aug 14 '24 13:08 0xnm

@0xnm I am not supposed to share the direct link of confluence here, so please search for the "[RFC] Session Replay Performance Metrics" for more detailed context.

Briefly speaking, the reason why interfaces are defined inside the core is because we need to use directly in our sdk code (core and features) to measure the function span. And the real implementations will be in tools/benchmark module.

So on client side, they are not supposed to import tools/benchmark module, so all the code we plant for the profiling will execute the No-op implementation. And all the interfaces are marked as @InternalApi in case of external usages.

On our side, the benchmark application depends on tools/benchmark, with GlobalBenchmark object, it is able to register the real implementation.

ambushwork avatar Aug 14 '24 14:08 ambushwork

PR is updated, now the interfaces are added into internal module.

ambushwork avatar Aug 28 '24 06:08 ambushwork