-
Notifications
You must be signed in to change notification settings - Fork 64
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
RUM-5553: Add DatadogMeter to read vital data for benchmark #2144
Conversation
19999f5
to
c3b1923
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## feature/metric-benchmark #2144 +/- ##
============================================================
+ Coverage 69.94% 69.97% +0.02%
============================================================
Files 726 726
Lines 26962 26962
Branches 4519 4519
============================================================
+ Hits 18858 18864 +6
+ Misses 6832 6830 -2
+ Partials 1272 1268 -4 |
66b7c54
to
47dbd9c
Compare
tools/benchmark/src/main/java/com/datadog/benchmark/DatadogMeter.kt
Outdated
Show resolved
Hide resolved
tools/benchmark/src/main/java/com/datadog/benchmark/internal/reader/CPUVitalReader.kt
Show resolved
Hide resolved
47dbd9c
to
1bd32a9
Compare
1bd32a9
to
0589a5d
Compare
@jonathanmos I just made the fix on the gauge as we discussed, please review it |
0589a5d
to
acf70d2
Compare
tools/benchmark/src/main/java/com/datadog/benchmark/exporter/LogsMetricExporter.kt
Show resolved
Hide resolved
*/ | ||
|
||
@Suppress("TooGenericExceptionCaught", "SwallowedException") | ||
private fun <T> File.safeCall( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we need these methods? they are originally were developed for the SDK usage, so that exception is not propagated to the client code. But if I get it right, this benchmark tooling is just internal tooling? So we probably don't care?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed, the whole file will be removed
lastFrameTime = currentFrameTime | ||
frameCount = 0 | ||
} | ||
Choreographer.getInstance().postFrameCallback(this) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why Choreographer is chosen over what androidx.metrics
gives?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't know androidx.metrics
can do the same thing, is there any advantage of android.metrics
comparing to Choreographer
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, a lot of advantages: it is a wrapper around different new APIs allowing collecting more granular metrics. Also by registering Choreographer
callback and not un-registering it when app goes into background/losing focus, you make CPU running more (this point probably doesn't matter a lot for internal tooling).
Anyway, FPS is not a good metric to judge about the performance and we were discussion to ditch it and replace it with something else (especially considering dynamic refresh rate). Much more meaningful metric will be something like the % of skipped/junky frames, for example.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good to know, I will first make a pull request to clean up the code on all the places where you mentioned, and another dedicated commit to replace the Choreographer
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall maybe for this work we shouldn't use FPS as a metric at all. But this is just my opinion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe you are right, with current synthetics test and metrics result, we can hardly see any difference between baseline and instrumented test, I will try to see if androidx.metrics
can provide some more valuable result.
private var lastFrameTime: Long = 0 | ||
private val intervalMs = FPS_SAMPLE_INTERVAL_IN_MS | ||
|
||
private val frameCallback = object : Choreographer.FrameCallback { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same question: why Choreographer
over androidx.metrics
?
What does this PR do?
Add DatadogMeter to read vital data for benchmark.
With this commit, the sample application can activate the
DatadogMeter
by adding this:Motivation
RUM-5553
Additional Notes
Some vital readers classes and unit tests are copied from feature modules for following reasons:
opentelemetry-java
repository, all the classes need to be independent from our SDK.Anything else we should know when reviewing?
Review checklist (to be filled by reviewers)