Skip to content

Update the Method Call metric usage#2040

Merged
xgouchet merged 1 commit into
developfrom
xgouchet/sr_perf
May 21, 2024
Merged

Update the Method Call metric usage#2040
xgouchet merged 1 commit into
developfrom
xgouchet/sr_perf

Conversation

@xgouchet

Copy link
Copy Markdown
Contributor

What does this PR do?

Update the MethodCall metric code to allow multiple usage with different operation names + cleanup some SR code

@xgouchet
xgouchet requested review from a team as code owners May 20, 2024 12:15
@xgouchet
xgouchet force-pushed the xgouchet/sr_perf branch 2 times, most recently from 9e9c2ac to 06f622e Compare May 20, 2024 14:59
@codecov-commenter

codecov-commenter commented May 20, 2024

Copy link
Copy Markdown

Codecov Report

Attention: Patch coverage is 77.69231% with 29 lines in your changes are missing coverage. Please review.

Project coverage is 83.03%. Comparing base (9117094) to head (f4f8507).

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #2040      +/-   ##
===========================================
- Coverage    83.23%   83.03%   -0.20%     
===========================================
  Files          491      493       +2     
  Lines        17685    17701      +16     
  Branches      2685     2685              
===========================================
- Hits         14719    14697      -22     
- Misses        2227     2271      +44     
+ Partials       739      733       -6     
Files Coverage Δ
...n/kotlin/com/datadog/android/api/InternalLogger.kt 100.00% <ø> (ø)
...in/com/datadog/android/api/feature/FeatureScope.kt 0.00% <ø> (ø)
...in/com/datadog/android/core/internal/SdkFeature.kt 89.10% <ø> (+2.77%) ⬆️
.../android/core/internal/logger/SdkInternalLogger.kt 85.71% <100.00%> (-2.20%) ⬇️
...oid/core/internal/metrics/MethodCalledTelemetry.kt 100.00% <100.00%> (ø)
.../datadog/android/core/metrics/PerformanceMetric.kt 0.00% <ø> (ø)
.../sessionreplay/internal/DefaultRecorderProvider.kt 92.31% <100.00%> (ø)
...nreplay/internal/recorder/SessionReplayRecorder.kt 96.52% <100.00%> (+0.09%) ⬆️
...essionreplay/internal/recorder/SnapshotProducer.kt 90.91% <100.00%> (+3.03%) ⬆️
...nreplay/internal/recorder/ViewOnDrawInterceptor.kt 88.89% <100.00%> (-1.59%) ⬇️
... and 4 more

... and 28 files with indirect coverage changes

* @param operationName the name of the operation being measured
* @return a PerformanceMetric object that can later be used to send telemetry, or null if sampled out
*/
fun startPerformanceMeasure(

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

side note: I added some comments to the original PR #1940 (review), but they were too late. I think in the future we should refactor this a bit a remove this method from the FeatureScope.

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.

Good point, I'll take your other comments here 👍

* @param samplingRate the sampling rate for the metric
* @param operation the operation to report
*/
fun <R : Any?> FeatureScope?.measureMethodCallPerf(

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
fun <R : Any?> FeatureScope?.measureMethodCallPerf(
@InternalApi
fun <R : Any?> FeatureScope?.measureMethodCallPerf(

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.

👍


val result = operation()

val isSuccessful = (result != null) && ((result !is Collection<*>) || result.isNotEmpty())

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

why generally speaking empty collection shouldn't be considered as a successful result? but probably for the methods we are going to cover with this call it makes sense.

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.

I was trying to keep the existing behavior consistent


dependencies {
api(project(":dd-sdk-android-core"))
api(project(":features:dd-sdk-android-trace"))

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

blocking: seems like unnecessary change? I don't see any com.datadog.android.trace or com.datadog.trace usages in this PR

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.

Indeed, I was using some traces in my tests, forgot to remove this

val nodes = views
.mapNotNull {
val nodes = sessionReplayFeature.measureMethodCallPerf(
javaClass,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

should this be in the consumer rules to prevent name obfuscation?

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.

Good point 👍

@xgouchet
xgouchet force-pushed the xgouchet/sr_perf branch from 06f622e to 7618a02 Compare May 21, 2024 12:03
@xgouchet
xgouchet requested a review from 0xnm May 21, 2024 12:04
Comment thread dd-sdk-android-core/api/apiSurface
@xgouchet
xgouchet force-pushed the xgouchet/sr_perf branch from 7618a02 to f4f8507 Compare May 21, 2024 12:48
@xgouchet
xgouchet merged commit a783738 into develop May 21, 2024
@xgouchet
xgouchet deleted the xgouchet/sr_perf branch May 21, 2024 13:18
@xgouchet xgouchet added this to the 2.10.x milestone Jul 31, 2024
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