Merged
Conversation
Contributor
Overall package sizeSelf size: 5.77 MB Dependency sizes
🤖 This report was automatically generated by heaviest-objects-in-the-universe |
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #3891 +/- ##
==========================================
- Coverage 84.60% 84.55% -0.06%
==========================================
Files 234 233 -1
Lines 9817 9713 -104
Branches 33 33
==========================================
- Hits 8306 8213 -93
+ Misses 1511 1500 -11 ☔ View full report in Codecov by Sentry. |
…ing end time of the current profile.
szegedi
force-pushed
the
szegedi/PROF-8829-fix-recording-times
branch
from
b24439e to
31bb4c1
Compare
szegedi
commented
Dec 20, 2023
szegedi
commented
Dec 20, 2023
BenchmarksBenchmark execution time: 2023-12-20 10:38:08 Comparing candidate commit 31bb4c1 in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 520 metrics, 12 unstable metrics. |
Qard
approved these changes
Dec 20, 2023
nsavoire
reviewed
Dec 20, 2023
| } else { | ||
| this._profiledIntervals += 1 | ||
| this._capture(this._timeoutInterval) | ||
| this._capture(this._timeoutInterval, new Date()) |
Collaborator
There was a problem hiding this comment.
It might be better to pass _lastDate here, since no new profile is started.
Collaborator
|
I wonder what is the impact of having inconsistent time information between pprof ( |
hoolioh
pushed a commit
that referenced
this pull request
Dec 21, 2023
* Ensure recording start time of next profile is the same as the recording end time of the current profile. * Pass the start-end range to profilers so they can use it * Use passed-in start-end instead of internal duration computations * Use end date to cut off late events for the next profile
Merged
hoolioh
pushed a commit
that referenced
this pull request
Dec 21, 2023
* Ensure recording start time of next profile is the same as the recording end time of the current profile. * Pass the start-end range to profilers so they can use it * Use passed-in start-end instead of internal duration computations * Use end date to cut off late events for the next profile
Merged
CarlesDD
pushed a commit
that referenced
this pull request
Dec 21, 2023
* Ensure recording start time of next profile is the same as the recording end time of the current profile. * Pass the start-end range to profilers so they can use it * Use passed-in start-end instead of internal duration computations * Use end date to cut off late events for the next profile
CarlesDD
pushed a commit
that referenced
this pull request
Dec 21, 2023
* Ensure recording start time of next profile is the same as the recording end time of the current profile. * Pass the start-end range to profilers so they can use it * Use passed-in start-end instead of internal duration computations * Use end date to cut off late events for the next profile
khanayan123
pushed a commit
that referenced
this pull request
Jan 2, 2024
* Ensure recording start time of next profile is the same as the recording end time of the current profile. * Pass the start-end range to profilers so they can use it * Use passed-in start-end instead of internal duration computations * Use end date to cut off late events for the next profile
khanayan123
pushed a commit
that referenced
this pull request
Jan 2, 2024
* Ensure recording start time of next profile is the same as the recording end time of the current profile. * Pass the start-end range to profilers so they can use it * Use passed-in start-end instead of internal duration computations * Use end date to cut off late events for the next profile
khanayan123
pushed a commit
that referenced
this pull request
Jan 2, 2024
* Ensure recording start time of next profile is the same as the recording end time of the current profile. * Pass the start-end range to profilers so they can use it * Use passed-in start-end instead of internal duration computations * Use end date to cut off late events for the next profile
khanayan123
pushed a commit
that referenced
this pull request
Jan 2, 2024
* Ensure recording start time of next profile is the same as the recording end time of the current profile. * Pass the start-end range to profilers so they can use it * Use passed-in start-end instead of internal duration computations * Use end date to cut off late events for the next profile
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Ensures subsequent profiles are emitted with back-to-back recording timestamps. The recording end time of a profile becomes the recording start time of the next profile.
Motivation
Before this change, the recording start time of the next profile would be considered to be the instant after profiles were submitted, while the recording end of the previous one was the instant just before profiles were gathered. This would result in the time period between the two to not be considered. While most of that period is spent serializing and submitting the profile (so, it's our library code) it contains async boundaries, so other work can also get scheduled.
It doesn't matter much for flame graphs and aggregate views, but the timeline only shows events between recording start time and recording end time, so it had the potential to not display samples, GC events etc. in the first few tens(!) of milliseconds (observed between 40-70ms.)
Additional Notes
I also enhanced the timeline events profiler to use the top-level profiler provided start and end times to emit better profile duration and time data, as well as to keep late events for the next profile. This latter state should not in fact occur as long as there's only synchronous processing between
_collectinprofiler.jscapturingnew Date()for the end date and the timeline profiler generating the profile, but I figured better be safe than sorry about missing events.Security
Datadog employees:
@DataDog/security-design-and-guidance.