dd-trace-java icon indicating copy to clipboard operation
dd-trace-java copied to clipboard

[DNM] JFR Context Integration PoC

Open jbachorik opened this issue 1 year ago • 3 comments

A trivial PoC to provide integration with the JFR Context prototype implementation.

How to build and use

  1. Clone https://github.com/DataDog/openjdk-jdk21/tree/jb/jfr_context_bp and build the JFR Context prototype OpenJDK 21
  2. Assuming that the custom OpenJDK build is in <path_to_openjdk> you can build the agent like JAVA_HOME=$JAVA_8_HOME ./gradlew -Pjfr_context.javac=<path_to_openjdk>/bin/javac :dd-java-agent:shadowJar
  3. Now you can use the custom dd-java-agent.jar found in dd-java-agent/build/libs/dd-java-agent-1.22.0-SNAPSHOT.jar to run the profiler on the patched openjdk to get the JFR events contextualized

Currently the following built-in events are contextualized:

  • jdk.ExecutionSample
  • jdk.NativeMethodSample
  • jdk.ObjectAllocationSample
  • jdk.ThreadPark
  • jdk.JavaMonitorEnter
  • jdk.JavaMonitorWait
  • jdk.JavaMonitorInflate
  • jdk.SystemGC
  • jdk.ObjectAllocationInNewTLAB
  • jdk.ObjectAllocationOutsideTLAB
  • jdk.ZAllocationStall
  • jdk.FileRead
  • jdk.FileWrite
  • jdk.SocketRead
  • jdk.SocketWrite
  • jdk.ThreadSleep

!Warning!

This is a very early prototype and the performance may not be as expected (mostly due to conversion between the trace/span id numbers to strings). Do not, I repeat, do not use this in production!!!

jbachorik avatar Oct 10 '23 15:10 jbachorik

@CodiumAI-Agent /review

jbachorik avatar Feb 06 '24 13:02 jbachorik

PR Analysis

(review updated until commit https://github.com/DataDog/dd-trace-java/commit/044feb3b0d3c3ccd0b74b6056dbe83c7d1c2cf9c)

  • 🎯 Main theme: Integration of Java Flight Recorder (JFR) Context with Datadog Profiling
  • 📝 PR summary: This PR is a proof of concept for integrating the JFR Context prototype implementation with Datadog Profiling. It modifies several classes to enable the use of JFR for profiling context labeling. It also includes changes to handle the initialization of the JFR context before JFR is initialized.
  • 📌 Type of PR: Enhancement
  • 🧪 Relevant tests added: No
  • ⏱️ Estimated effort to review [1-5]: 4, because the PR involves changes in multiple files and the introduction of a new concept (JFR Context), which requires a good understanding of both the existing codebase and the new feature.
  • 🔒 Security concerns: No security concerns found

PR Feedback

💡 General suggestions: The PR is a good start for integrating JFR Context into the Datadog Profiling. However, it's important to add tests to validate the new functionality and ensure it doesn't break existing features. Also, the PR should be marked as a draft if it's not ready for production use. Lastly, it would be beneficial to add more comments in the code to explain the purpose and functionality of the new code.

🤖 Code feedback:
relevant filedd-java-agent/agent-profiling/profiling-uploader/src/main/java/com/datadog/profiling/uploader/ProfileUploader.java
suggestion      

The latch.countDown(); calls have been commented out in the onResponse and onFailure methods. If these are not needed, they should be removed completely. If they are needed in the future, a comment should be added to explain why they are currently commented out. [important]

relevant line// latch.countDown();

relevant filedd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/Agent.java
suggestion      

The ProfilingContextIntegration ddprof is now initialized to ProfilingContextIntegration.NoOp.INSTANCE before the if condition. If the condition is not met, the ddprof remains as NoOp.INSTANCE. This change seems unnecessary as the previous code was more concise and achieved the same result. [medium]

relevant lineProfilingContextIntegration ddprof = ProfilingContextIntegration.NoOp.INSTANCE;

relevant filedd-java-agent/agent-profiling/profiling-jfr-context/src/main/java/com/datadog/profiling/jfr/context/ContextIntegration.java
suggestion      

The initialize method sets the profilingContextAccess to a new ProfilerContext.Access instance. It would be better to check if profilingContextAccess is already set before creating a new instance to avoid unnecessary object creation. [medium]

relevant lineDDSpanContext.profilingContextAccess = new ProfilerContext.Access() {


✨ Usage guide:

Overview: The review tool scans the PR code changes, and generates a PR review. The tool can be triggered automatically every time a new PR is opened, or can be invoked manually by commenting on any PR. When commenting, to edit configurations related to the review tool (pr_reviewer section), use the following template:

/review --pr_reviewer.some_config1=... --pr_reviewer.some_config2=...

With a configuration file, use the following template:

[pr_reviewer]
some_config1=...
some_config2=...
Utilizing extra instructions

The review tool can be configured with extra instructions, which can be used to guide the model to a feedback tailored to the needs of your project.

Be specific, clear, and concise in the instructions. With extra instructions, you are the prompter. Specify the relevant sub-tool, and the relevant aspects of the PR that you want to emphasize.

Examples for extra instructions:

[pr_reviewer] # /review #
extra_instructions="""
In the 'general suggestions' section, emphasize the following:
- Does the code logic cover relevant edge cases?
- Is the code logic clear and easy to understand?
- Is the code logic efficient?
...
"""

Use triple quotes to write multi-line instructions. Use bullet points to make the instructions more readable.

How to enable\disable automation
  • When you first install PR-Agent app, the default mode for the review tool is:
pr_commands = ["/review", ...]

meaning the review tool will run automatically on every PR, with the default configuration. Edit this field to enable/disable the tool, or to change the used configurations

Auto-labels

The review tool can auto-generate two specific types of labels for a PR:

  • a possible security issue label, that detects possible security issues (enable_review_labels_security flag)
  • a Review effort [1-5]: x label, where x is the estimated effort to review the PR (enable_review_labels_effort flag)
Extra sub-tools

The review tool provides a collection of possible feedbacks about a PR. It is recommended to review the possible options, and choose the ones relevant for your use case. Some of the feature that are disabled by default are quite useful, and should be considered for enabling. For example: require_score_review, require_soc2_ticket, and more.

More PR-Agent commands

To invoke the PR-Agent, add a comment using one of the following commands:

  • /review: Request a review of your Pull Request.
  • /describe: Update the PR title and description based on the contents of the PR.
  • /improve [--extended]: Suggest code improvements. Extended mode provides a higher quality feedback.
  • /ask <QUESTION>: Ask a question about the PR.
  • /update_changelog: Update the changelog based on the PR's contents.
  • /add_docs 💎: Generate docstring for new components introduced in the PR.
  • /generate_labels 💎: Generate labels for the PR based on the PR's contents.
  • /analyze 💎: Automatically analyzes the PR, and presents changes walkthrough for each component.

See the tools guide for more details. To list the possible configuration parameters, add a /config comment.

See the review usage page for a comprehensive guide on using this tool.

CodiumAI-Agent avatar Feb 06 '24 13:02 CodiumAI-Agent

Persistent review updated to latest commit https://github.com/DataDog/dd-trace-java/commit/044feb3b0d3c3ccd0b74b6056dbe83c7d1c2cf9c

CodiumAI-Agent avatar Feb 06 '24 13:02 CodiumAI-Agent