Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Profiler API to record time spent in queues #4653

Merged
merged 1 commit into from
Feb 7, 2023
Merged

Profiler API to record time spent in queues #4653

merged 1 commit into from
Feb 7, 2023

Conversation

richardstartin
Copy link
Member

What Does This Do

Motivation

Additional Notes

@richardstartin richardstartin changed the title use continutations to record queuing time between scheduling and execution use continuations to record queuing time between scheduling and execution Feb 3, 2023
@richardstartin richardstartin force-pushed the rgs/queueing-time branch 3 times, most recently from e030299 to 03bbf09 Compare February 3, 2023 19:44
richardstartin
richardstartin commented Feb 3, 2023
View reviewed changes
...e-core/src/test/groovy/datadog/trace/core/scopemanager/ScopeAndContinuationLayoutTest.groovy Outdated
@@ -14,11 +14,11 @@ class ScopeAndContinuationLayoutTest extends DDSpecification {
}

def "single continuation layout"() {
expect: layoutAcceptable(ContinuableScopeManager.SingleContinuation, 32)
expect: layoutAcceptable(ContinuableScopeManager.SingleContinuation, 40)
Copy link
Member Author

Choose a reason for hiding this comment

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

this could stay at 32 if, for example, the reference to AgentTrace were removed. We can get away with just 32 bits for the timestamp because we know the queuing duration will be less than 25 days.

Copy link
Contributor

Choose a reason for hiding this comment

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

How would you make Continuation work without an AgentTrace instance?
For the record, I replace it by a Context instance in my PoC.

Copy link
Member Author

Choose a reason for hiding this comment

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

there's a reference chain from the span to the trace, so a couple of indirections can be traded for a reduction in space.

@richardstartin richardstartin added the comp: profiling Profiling label Feb 3, 2023
@richardstartin richardstartin force-pushed the rgs/queueing-time branch 13 times, most recently from ed46ed5 to 612dd50 Compare February 6, 2023 19:09
@richardstartin richardstartin marked this pull request as ready for review February 7, 2023 08:56
@richardstartin richardstartin requested review from a team as code owners February 7, 2023 08:56
PerfectSlayer
PerfectSlayer reviewed Feb 7, 2023
View reviewed changes
...e-core/src/test/groovy/datadog/trace/core/scopemanager/ScopeAndContinuationLayoutTest.groovy Outdated
@@ -14,11 +14,11 @@ class ScopeAndContinuationLayoutTest extends DDSpecification {
}

def "single continuation layout"() {
expect: layoutAcceptable(ContinuableScopeManager.SingleContinuation, 32)
expect: layoutAcceptable(ContinuableScopeManager.SingleContinuation, 40)
Copy link
Contributor

Choose a reason for hiding this comment

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

How would you make Continuation work without an AgentTrace instance?
For the record, I replace it by a Context instance in my PoC.

dd-trace-core/src/main/java/datadog/trace/core/scopemanager/ContinuableScopeManager.java Outdated
Comment on lines 686 to 687
AgentScope scope = scopeManager.continueSpan(this, spanUnderScope, source);
scopeManager.recordContinuationLifeSpan(timestampMillis);
Copy link
Contributor

@PerfectSlayer PerfectSlayer Feb 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Any implicit reason to call ScopeManager.continueSpan() before ScopeManager.recordContinuationLifeSpan()?
(by implicit, I mean I could not find one but I expect to missed it 😓 )

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, to ensure the scope is on the stack (which has the side effect of activating it) - this probably requires a comment, so thanks for picking up on it.

Copy link
Member Author

Choose a reason for hiding this comment

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

as discussed offline, I think we can wait for changes in this area and keep this PR scoped to the profiler changes

@richardstartin richardstartin changed the title use continuations to record queuing time between scheduling and execution Profiler API to record time spent in queues Feb 7, 2023
@richardstartin richardstartin merged commit 571e51e into master Feb 7, 2023
@richardstartin richardstartin deleted the rgs/queueing-time branch February 7, 2023 16:48
@github-actions github-actions bot added this to the 1.7.0 milestone Feb 7, 2023
@richardstartin richardstartin added the tag: no release notes Changes to exclude from release notes label Feb 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@PerfectSlayer PerfectSlayer PerfectSlayer left review comments

@jbachorik jbachorik jbachorik approved these changes

Assignees
No one assigned
Labels
comp: profiling Profiling tag: no release notes Changes to exclude from release notes
Projects
None yet
Milestone
1.7.0
Development

Successfully merging this pull request may close these issues.
3 participants
@richardstartin @jbachorik @PerfectSlayer