-
Notifications
You must be signed in to change notification settings - Fork 343
Add wallclock TaskBlock profiler support #11544
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
base: master
Are you sure you want to change the base?
Changes from all commits
a446cd6
d14fbc1
34e6014
69c92df
1cb6a9b
6ed3220
9feb87a
dc50095
df2f481
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -68,6 +68,10 @@ variables: | |
| description: "Enable flaky tests" | ||
| value: "false" | ||
|
|
||
| JAVA_PROFILER_REF: | ||
| description: "When non-empty, clone DataDog/java-profiler at this Git ref (branch or tag), build ddprof, and use it as ddprof.jar for Gradle jobs instead of the Maven dependency." | ||
| value: "paul.fournillon/wallclock-taskblock" | ||
|
|
||
| # One pipeline injection package size ratchet | ||
| OCI_PACKAGE_MAX_SIZE_BYTES: 40_000_000 | ||
| LIB_INJECTION_IMAGE_MAX_SIZE_BYTES: 40_000_000 | ||
|
|
@@ -197,9 +201,21 @@ default: | |
| echo "Failed to find base ref for PR" >&2 | ||
| fi | ||
|
|
||
| # When build_java_profiler_ddprof ran, its artifact is available at custom-ddprof/ddprof.jar. | ||
| # Append root project property expected by dd-java-agent/ddprof-lib/build.gradle. | ||
| .inject_custom_ddprof_jar: &inject_custom_ddprof_jar | ||
| - | | ||
| if [ -f "${CI_PROJECT_DIR}/custom-ddprof/ddprof.jar" ]; then | ||
| echo "ddprof.jar=${CI_PROJECT_DIR}/custom-ddprof/ddprof.jar" >> gradle.properties | ||
| echo "Using custom ddprof.jar from java-profiler build" | ||
| fi | ||
|
|
||
| .gradle_build: &gradle_build | ||
| image: ${BUILDER_IMAGE_REPO}:${BUILDER_IMAGE_VERSION_PREFIX}base | ||
| stage: build | ||
| needs: | ||
| - job: build_java_profiler_ddprof | ||
| optional: true | ||
| variables: | ||
| <<: *tier_m_variables | ||
| MAVEN_OPTS: "-Xms256M -Xmx1024M" | ||
|
|
@@ -245,6 +261,7 @@ default: | |
| org.gradle.java.installations.fromEnv=$JAVA_HOMES | ||
| org.gradle.console=colored | ||
| EOF | ||
| - *inject_custom_ddprof_jar | ||
| - mkdir -p .gradle | ||
| - export GRADLE_USER_HOME=$(pwd)/.gradle | ||
| # Apache Maven Wrapper supports MVNW_REPOURL for repository-manager downloads: | ||
|
|
@@ -328,8 +345,114 @@ dd-octo-sts-pre-release-check: | |
| max: 2 | ||
| when: always | ||
|
|
||
| # Builds java-profiler from JAVA_PROFILER_REF and publishes custom-ddprof/ddprof.jar for downstream Gradle jobs. | ||
| # Uses :ddprof-lib:assembleReleaseJar (not assembleRelease, which is native-only). JDK 21+ for release + JDK 17+ for Gradle 9. | ||
| build_java_profiler_ddprof: | ||
| image: ${BUILDER_IMAGE_REPO}:${BUILDER_IMAGE_VERSION_PREFIX}base | ||
| stage: build | ||
| rules: | ||
| - if: '$JAVA_PROFILER_REF =~ /.+/' | ||
| when: on_success | ||
| variables: | ||
| FF_USE_FASTZIP: "true" | ||
| CACHE_COMPRESSION_LEVEL: "slowest" | ||
| KUBERNETES_CPU_REQUEST: 10 | ||
| KUBERNETES_MEMORY_REQUEST: 20Gi | ||
| KUBERNETES_MEMORY_LIMIT: 20Gi | ||
| before_script: | ||
| - | | ||
| # java-profiler uses Gradle 9.x; Gradle requires JVM 17+. Builder image default java is often JDK 8. | ||
| if [ -n "${JAVA_21_HOME:-}" ] && [ -x "${JAVA_21_HOME}/bin/java" ]; then | ||
| export JAVA_HOME="$JAVA_21_HOME" | ||
| elif [ -n "${JAVA_17_HOME:-}" ] && [ -x "${JAVA_17_HOME}/bin/java" ]; then | ||
| export JAVA_HOME="$JAVA_17_HOME" | ||
| else | ||
| shopt -s nullglob | ||
| for d in /usr/lib/jvm/java-21-* /usr/lib/jvm/temurin-21-* /usr/lib/jvm/java-17-*; do | ||
| if [ -x "${d}/bin/java" ]; then | ||
| export JAVA_HOME="$d" | ||
| break | ||
| fi | ||
| done | ||
| shopt -u nullglob | ||
| fi | ||
| if [ -z "${JAVA_HOME:-}" ] || ! [ -x "${JAVA_HOME}/bin/java" ]; then | ||
| echo "Could not find JDK 17+ for Gradle 9 (set JAVA_21_HOME or JAVA_17_HOME, or install JDK 21 under /usr/lib/jvm)." >&2 | ||
| ls -la /usr/lib/jvm 2>/dev/null || true | ||
| exit 1 | ||
| fi | ||
| export PATH="${JAVA_HOME}/bin:${PATH}" | ||
| java -version | ||
| script: | ||
| - | | ||
| set -euo pipefail | ||
| mkdir -p "${CI_PROJECT_DIR}/custom-ddprof" | ||
| SRCDIR="${CI_PROJECT_DIR}/java-profiler-src" | ||
| rm -rf "$SRCDIR" | ||
| git clone --depth 1 --branch "$JAVA_PROFILER_REF" https://github.com/DataDog/java-profiler.git "$SRCDIR" | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Sphinx Review — MEDIUM] External java-profiler clone is pinned only to a mutable branch ref (--branch $JAVA_PROFILER_REF --depth 1) with no commit SHA. The produced ddprof.jar is injected into downstream jobs including deploy_to_maven_central and deploy_snapshot_with_ddprof_snapshot, with no reproducibility or supply-chain integrity guarantee. Any force-push to the branch silently changes the artifact without a CI diff. Suggestion: Require a commit SHA (or signed tag) rather than a branch for the ref, or restrict the custom-build path to non-publish pipelines. At minimum, exclude build_java_profiler_ddprof from deploy_to_maven_central / deploy_snapshot needs so a custom build can never reach a release. [concern: security · confidence: medium]
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. same comment as above! |
||
| cd "$SRCDIR" | ||
| export ORG_GRADLE_PROJECT_mavenRepositoryProxy="$MAVEN_REPOSITORY_PROXY" | ||
| export ORG_GRADLE_PROJECT_gradlePluginProxy="$GRADLE_PLUGIN_PROXY" | ||
| PROFILER_GRADLE_INIT="${CI_PROJECT_DIR}/java-profiler-init.gradle" | ||
| cat > "$PROFILER_GRADLE_INIT" <<'EOF' | ||
| def mavenRepositoryProxy = System.getenv('MAVEN_REPOSITORY_PROXY') | ||
| def gradlePluginProxy = System.getenv('GRADLE_PLUGIN_PROXY') ?: mavenRepositoryProxy | ||
|
|
||
| def addPluginRepositories = { repositories -> | ||
| if (gradlePluginProxy) { | ||
| repositories.maven { url = uri(gradlePluginProxy) } | ||
| } | ||
| if (mavenRepositoryProxy && mavenRepositoryProxy != gradlePluginProxy) { | ||
| repositories.maven { url = uri(mavenRepositoryProxy) } | ||
| } | ||
| } | ||
|
|
||
| def addMavenRepositories = { repositories -> | ||
| if (mavenRepositoryProxy) { | ||
| repositories.maven { url = uri(mavenRepositoryProxy) } | ||
| } | ||
| } | ||
|
|
||
| beforeSettings { settings -> | ||
| settings.pluginManagement { | ||
| repositories { | ||
| addPluginRepositories(delegate) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| allprojects { | ||
| buildscript { | ||
| repositories { | ||
| addPluginRepositories(delegate) | ||
| } | ||
| } | ||
| repositories { | ||
| addMavenRepositories(delegate) | ||
| } | ||
| } | ||
| EOF | ||
| chmod +x ./gradlew | ||
| ./gradlew --version | ||
| # assembleRelease is the native link/assemble task only; the packaged jar is assembleReleaseJar. | ||
| ./gradlew --init-script "$PROFILER_GRADLE_INIT" :ddprof-lib:assembleReleaseJar -Pskip-tests -Pskip-gtest | ||
| JAR=$(find ddprof-lib/build/libs -maxdepth 1 -type f \( -name 'ddprof-*.jar' \) ! -name '*-sources*' ! -name '*-javadoc*' | head -1) | ||
| if [ -z "$JAR" ] || [ ! -f "$JAR" ]; then | ||
| echo "No ddprof jar found under ddprof-lib/build/libs" >&2 | ||
| ls -la ddprof-lib/build/libs 2>/dev/null || ls -laR ddprof-lib/build 2>/dev/null || true | ||
| exit 1 | ||
| fi | ||
| cp "$JAR" "${CI_PROJECT_DIR}/custom-ddprof/ddprof.jar" | ||
| ls -la "${CI_PROJECT_DIR}/custom-ddprof/" | ||
| artifacts: | ||
| when: on_success | ||
| paths: | ||
| - custom-ddprof/ddprof.jar | ||
|
|
||
| build: | ||
| needs: | ||
| - job: build_java_profiler_ddprof | ||
| optional: true | ||
| - job: maven-central-pre-release-check | ||
| optional: true | ||
| - job: dd-octo-sts-pre-release-check | ||
|
|
@@ -441,7 +564,9 @@ publish-artifacts-to-s3: | |
| spotless: | ||
| extends: .gradle_build | ||
| stage: tests | ||
| needs: [] | ||
| needs: | ||
| - job: build_java_profiler_ddprof | ||
| optional: true | ||
| variables: | ||
| GRADLE_MEMORY_MAX: 6G | ||
| CACHE_TYPE: "spotless" | ||
|
|
@@ -454,15 +579,19 @@ spotless: | |
| check-instrumentation-naming: | ||
| extends: .gradle_build | ||
| stage: tests | ||
| needs: [ ] | ||
| needs: | ||
| - job: build_java_profiler_ddprof | ||
| optional: true | ||
| script: | ||
| - ./gradlew --version | ||
| - ./gradlew checkInstrumentationNaming | ||
|
|
||
| config-inversion-linter: | ||
| extends: .gradle_build | ||
| stage: tests | ||
| needs: [] | ||
| needs: | ||
| - job: build_java_profiler_ddprof | ||
| optional: true | ||
| script: | ||
| - ./gradlew --version | ||
| - ./gradlew checkConfigurations | ||
|
|
@@ -471,7 +600,10 @@ test_published_artifacts: | |
| extends: .gradle_build | ||
| image: ${BUILDER_IMAGE_REPO}:${BUILDER_IMAGE_VERSION_PREFIX}7 # Needs Java7 for some tests | ||
| stage: tests | ||
| needs: [ build ] | ||
| needs: | ||
| - job: build_java_profiler_ddprof | ||
| optional: true | ||
| - build | ||
| variables: | ||
| CACHE_TYPE: "lib" | ||
| script: | ||
|
|
@@ -499,7 +631,10 @@ test_published_artifacts: | |
|
|
||
| .check_job: | ||
| extends: .gradle_build | ||
| needs: [ build ] | ||
| needs: | ||
| - job: build_java_profiler_ddprof | ||
| optional: true | ||
| - build | ||
| stage: tests | ||
| variables: | ||
| CACHE_TYPE: "lib" | ||
|
|
@@ -535,7 +670,9 @@ test_published_artifacts: | |
|
|
||
| check_build_src: | ||
| extends: .check_job | ||
| needs: [] | ||
| needs: | ||
| - job: build_java_profiler_ddprof | ||
| optional: true | ||
| variables: | ||
| GRADLE_TARGET: ":buildSrc:build" | ||
|
|
||
|
|
@@ -574,6 +711,8 @@ muzzle: | |
| # Keep matrix vars exact and in build_tests declaration order: | ||
| # https://docs.gitlab.com/ci/yaml/#needsparallelmatrix | ||
| needs: &needs_build_tests_inst | ||
| - job: build_java_profiler_ddprof | ||
| optional: true | ||
| - job: build_tests | ||
| parallel: | ||
| matrix: | ||
|
|
@@ -659,7 +798,10 @@ muzzle-dep-report: | |
| extends: .gradle_build | ||
| image: ${BUILDER_IMAGE_REPO}:${BUILDER_IMAGE_VERSION_PREFIX}$testJvm | ||
| tags: [ "docker-in-docker:amd64" ] # use docker-in-docker runner for testcontainers | ||
| needs: [ build_tests ] | ||
| needs: | ||
| - job: build_java_profiler_ddprof | ||
| optional: true | ||
| - build_tests | ||
| stage: tests | ||
| variables: | ||
| <<: *tier_m_variables | ||
|
|
@@ -1262,7 +1404,10 @@ deploy_to_di_backend:manual: | |
| deploy_to_maven_central: | ||
| extends: .gradle_build | ||
| stage: publish | ||
| needs: [ build ] | ||
| needs: | ||
| - job: build_java_profiler_ddprof | ||
| optional: true | ||
| - build | ||
| variables: | ||
| CACHE_TYPE: "lib" | ||
| rules: | ||
|
|
@@ -1291,7 +1436,10 @@ deploy_to_maven_central: | |
| deploy_snapshot_with_ddprof_snapshot: | ||
| extends: .gradle_build | ||
| stage: publish | ||
| needs: [ build ] | ||
| needs: | ||
| - job: build_java_profiler_ddprof | ||
| optional: true | ||
| - build | ||
| variables: | ||
| CACHE_TYPE: "lib" | ||
| rules: | ||
|
|
||
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.
[Sphinx Review — HIGH] JAVA_PROFILER_REF defaults to a personal feature branch ('paul.fournillon/wallclock-taskblock'). The build_java_profiler_ddprof job fires whenever the value is non-empty, so by default the entire pipeline builds and injects ddprof from this personal branch instead of the published artifact. This includes deploy_to_maven_central and deploy_snapshot_with_ddprof_snapshot. If the personal branch is deleted, force-pushed, or renamed, 'git clone --branch' fails and breaks all builds.
Suggestion: Set the default value to an empty string before merge (keep the override mechanism for ad-hoc testing). Apply the same change to env JAVA_PROFILER_REF in .github/workflows/run-system-tests.yaml.
[CONSENSUS · confidence: high · adversary-tested]
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.
it is definitely not something we want to keep in the end. I've made these temporary changes so the CI tests can run smoothly while working on this PR: it avoids facing build issues that can hide real regression bugs. Before this PR is merged, both
.gitlab-ci.ymland.github/workflowswill be reset to their original state.