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

implement checkpoint API for profiler integration #2687

Merged
merged 1 commit into from
May 6, 2021
Merged

implement checkpoint API for profiler integration #2687

merged 1 commit into from
May 6, 2021

Conversation

richardstartin
Copy link
Member

@richardstartin richardstartin commented May 5, 2021 •
edited

Captures span start, finish and thread migration, allows instrumentation to trigger recording of CPU intensive work.

Mutually exclusive with scope events. Activated by -Ddd.profiling.legacy.tracing.integration=false (which disables scope events).

@richardstartin richardstartin requested a review from a team as a code owner May 5, 2021 13:06
tylerbenson
tylerbenson reviewed May 5, 2021
View reviewed changes
dd-trace-core/src/test/groovy/datadog/trace/core/PendingTraceTestBase.groovy
// this test is kept close to pending trace despite not exercising
// its methods directly because this is where the reponsibility lies
// for emitting the checkpoints, and this is the least surprising place
// for a test to fail when modifying PendingTrace
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the comment.

dd-trace-core/src/main/java/datadog/trace/core/PendingTrace.java
ROOT_SPAN.compareAndSet(this, null, span);
PENDING_REFERENCE_COUNT.incrementAndGet(this);
}

void addFinishedSpan(final DDSpan span) {
tracer.onFinish(span);
Copy link
Contributor

Choose a reason for hiding this comment

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

Are these new methods going to be added to the instrumentation or is PendingTrace sufficient?

Copy link
Member Author

Choose a reason for hiding this comment

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

Instrumentation can be augmented to record CPU bound work, but I haven't found a concrete use case for this not already modelled with a synchronous span. Span start/finish are handled automatically. Thread migrations (from which queuing/idle time can be derived) are handled automatically, but may need to be patched in some instrumentations in cases where the Thread API is used by the library and we're not using continuations for context propagation, so unlikely.

@richardstartin richardstartin added the comp: profiling Profiling label May 5, 2021
jbachorik
jbachorik reviewed May 5, 2021
View reviewed changes
Copy link
Contributor

@jbachorik jbachorik left a comment

Choose a reason for hiding this comment

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

Looks good!
I have only some minor comments about the JFR event definition.

dd-trace-core/jfr-openjdk/src/main/java/datadog/trace/core/jfr/openjdk/CheckpointEvent.java Outdated Show resolved Hide resolved
dd-trace-core/jfr-openjdk/src/main/java/datadog/trace/core/jfr/openjdk/CheckpointEvent.java Outdated Show resolved Hide resolved
dd-trace-core/jfr-openjdk/src/main/java/datadog/trace/core/jfr/openjdk/JFRCheckpointer.java Outdated Show resolved Hide resolved
dd-trace-core/src/main/java/datadog/trace/core/DDSpanContext.java
@@ -349,7 +349,6 @@ public PendingTrace getTrace() {
return trace;
}

@Deprecated
Copy link
Contributor

Choose a reason for hiding this comment

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

😸

internal-api/src/main/java/datadog/trace/api/Checkpointer.java
Comment on lines +5 to +8
/**
* Modifier tag to make an event an end event. The LSB is chosen to allow for good varint
* compression.
*/
Copy link
Contributor

Choose a reason for hiding this comment

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

🔥

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

Copy link
Member Author

Choose a reason for hiding this comment

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

🤓

bantonsson
bantonsson approved these changes May 5, 2021
View reviewed changes
Copy link
Contributor

@bantonsson bantonsson left a comment

Choose a reason for hiding this comment

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

Nice. The only thing that feels weird is the imbalance in the onThreadMigration and onAsyncResume method names. Would be nice if they lined up better.

@richardstartin
Copy link
Member Author

richardstartin commented May 5, 2021 •
edited

The only thing that feels weird is the imbalance in the onThreadMigration and onAsyncResume method names. Would be nice if they lined up better.

Any ideas? onCapture/onActivate?

I have some ideas.

@bantonsson
Copy link
Contributor

Consistent ...ThreadMigration or maybe just ...Migration is fine.

@richardstartin
Copy link
Member Author

For naming consistency, I use startX and finishX everywhere, including for thread migrations, which are now started when continuations are captured and finished when continuations are activated.

jbachorik
jbachorik approved these changes May 6, 2021
View reviewed changes
Copy link
Contributor

@jbachorik jbachorik left a comment

Choose a reason for hiding this comment

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

LGTM!

@richardstartin richardstartin merged commit bd47660 into master May 6, 2021
@richardstartin richardstartin deleted the rgs/checkpoints branch May 6, 2021 08:24
@github-actions github-actions bot added this to the 0.80.0 milestone May 6, 2021
@richardstartin richardstartin mentioned this pull request May 6, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tylerbenson tylerbenson tylerbenson left review comments

@jbachorik jbachorik jbachorik approved these changes

@bantonsson bantonsson bantonsson approved these changes

Assignees
No one assigned
Labels
comp: profiling Profiling
Projects
None yet
Milestone
0.80.0
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@richardstartin @bantonsson @tylerbenson @jbachorik