bazel/sqlite3: share the libsqlite3 build with mattn/go-sqlite3 - #50749
bazel/sqlite3: share the libsqlite3 build with mattn/go-sqlite3#50749chouquette wants to merge 9 commits into
Conversation
chouquette
added
changelog/no-changelog
97b5669 to
a33f852
Compare
Files inventory check summaryFile checks results against ancestor 89977ff5: Results for datadog-agent_7.81.0~devel.git.655.021274d.pipeline.118049289-1_amd64.deb:Detected file changes:
|
Static quality checks❌ Please find below the results from static quality gates Error
Gate failure full details
Static quality gates prevent the PR to merge! Successful checksInfo
18 successful checks with minimal change (< 2 KiB)
|
Regression DetectorRegression Detector ResultsMetrics dashboard Baseline: 89977ff Optimization Goals: ✅ No significant changes detected
|
| perf | experiment | goal | Δ mean % | Δ mean % CI | trials | links |
|---|---|---|---|---|---|---|
| ➖ | quality_gate_logs | % cpu utilization | +0.44 | [-0.63, +1.50] | 1 | Logs bounds checks dashboard |
| ➖ | quality_gate_idle | memory utilization | +0.32 | [+0.26, +0.38] | 1 | Logs bounds checks dashboard |
| ➖ | quality_gate_metrics_logs | memory utilization | +0.31 | [+0.04, +0.57] | 1 | Logs bounds checks dashboard |
| ➖ | quality_gate_idle_all_features | memory utilization | +0.11 | [+0.07, +0.15] | 1 | Logs bounds checks dashboard |
Bounds Checks: ✅ Passed
| perf | experiment | bounds_check_name | replicates_passed | observed_value | links |
|---|---|---|---|---|---|
| ✅ | quality_gate_idle | intake_connections | 10/10 | 3 ≤ 4 | bounds checks dashboard |
| ✅ | quality_gate_idle | memory_usage | 10/10 | 145.97MiB ≤ 147MiB | bounds checks dashboard |
| ✅ | quality_gate_idle | total_bytes_received | 10/10 | 731.39KiB ≤ 819.20KiB | bounds checks dashboard |
| ✅ | quality_gate_idle_all_features | intake_connections | 10/10 | 3 ≤ 4 | bounds checks dashboard |
| ✅ | quality_gate_idle_all_features | memory_usage | 10/10 | 484.20MiB ≤ 495MiB | bounds checks dashboard |
| ✅ | quality_gate_idle_all_features | total_bytes_received | 10/10 | 1.13MiB ≤ 1.25MiB | bounds checks dashboard |
| ✅ | quality_gate_logs | intake_connections | 10/10 | 3 ≤ 6 | bounds checks dashboard |
| ✅ | quality_gate_logs | memory_usage | 10/10 | 180.93MiB ≤ 195MiB | bounds checks dashboard |
| ✅ | quality_gate_logs | missed_bytes | 10/10 | 0B = 0B | bounds checks dashboard |
| ✅ | quality_gate_logs | total_bytes_received | 10/10 | 264.04MiB ≤ 292MiB | bounds checks dashboard |
| ✅ | quality_gate_metrics_logs | cpu_usage | 10/10 | 349.69 ≤ 2000 | bounds checks dashboard |
| ✅ | quality_gate_metrics_logs | intake_connections | 10/10 | 4 ≤ 6 | bounds checks dashboard |
| ✅ | quality_gate_metrics_logs | memory_usage | 10/10 | 405.51MiB ≤ 430MiB | bounds checks dashboard |
| ✅ | quality_gate_metrics_logs | missed_bytes | 10/10 | 0B = 0B | bounds checks dashboard |
| ✅ | quality_gate_metrics_logs | total_bytes_received | 10/10 | 0.93GiB ≤ 1.04GiB | bounds checks dashboard |
Explanation
Confidence level: 90.00%
Effect size tolerance: |Δ mean %| ≥ 5.00%
Performance changes are noted in the perf column of each table:
- ✅ = significantly better comparison variant performance
- ❌ = significantly worse comparison variant performance
- ➖ = no significant change in performance
A regression test is an A/B test of target performance in a repeatable rig, where "performance" is measured as "comparison variant minus baseline variant" for an optimization goal (e.g., ingress throughput). Due to intrinsic variability in measuring that goal, we can only estimate its mean value for each experiment; we report uncertainty in that value as a 90.00% confidence interval denoted "Δ mean % CI".
For each experiment, we decide whether a change in performance is a "regression" -- a change worth investigating further -- if all of the following criteria are true:
-
Its estimated |Δ mean %| ≥ 5.00%, indicating the change is big enough to merit a closer look.
-
Its 90.00% confidence interval "Δ mean % CI" does not contain zero, indicating that if our statistical model is accurate, there is at least a 90.00% chance there is a difference in performance between baseline and comparison variants.
-
Its configuration does not mark it "erratic".
Replicate Execution Details
We run multiple replicates for each experiment/variant. However, we allow replicates to be automatically retried if there are any failures, up to 8 times, at which point the replicate is marked dead and we are unable to run analysis for the entire experiment. We call each of these attempts at running replicates a replicate execution. This section lists all replicate executions that failed due to the target crashing or being oom killed.
Note: In the below tables we bucket failures by experiment, variant, and failure type. For each of these buckets we list out the replicate indexes that failed with an annotation signifying how many times said replicate failed with the given failure mode. In the below example the baseline variant of the experiment named experiment_with_failures had two replicates that failed by oom kills. Replicate 0, which failed 8 executions, and replicate 1 which failed 6 executions, all with the same failure mode.
| Experiment | Variant | Replicates | Failure | Logs | Debug Dashboard |
|---|---|---|---|---|---|
| experiment_with_failures | baseline | 0 (x8) 1 (x6) | Oom killed | Debug Dashboard |
The debug dashboard links will take you to a debugging dashboard specifically designed to investigate replicate execution failures.
❌ Retried Profiling Replicate Execution Failures (ddprof)
Note: Profiling replicas may still be executing. See the debug dashboard for up to date status.
| Experiment | Variant | Replicates | Failure | Debug Dashboard |
|---|---|---|---|---|
| quality_gate_idle_all_features | baseline | 10 | Oom killed | Debug Dashboard |
| quality_gate_idle_all_features | comparison | 10 | Oom killed | Debug Dashboard |
| quality_gate_logs | baseline | 10 | Oom killed | Debug Dashboard |
| quality_gate_metrics_logs | baseline | 10 | Oom killed | Debug Dashboard |
| quality_gate_metrics_logs | comparison | 10 | Oom killed | Debug Dashboard |
CI Pass/Fail Decision
✅ Passed. All Quality Gates passed.
- quality_gate_logs, bounds check memory_usage: 10/10 replicas passed. Gate passed.
- quality_gate_logs, bounds check total_bytes_received: 10/10 replicas passed. Gate passed.
- quality_gate_logs, bounds check intake_connections: 10/10 replicas passed. Gate passed.
- quality_gate_logs, bounds check missed_bytes: 10/10 replicas passed. Gate passed.
- quality_gate_metrics_logs, bounds check cpu_usage: 10/10 replicas passed. Gate passed.
- quality_gate_metrics_logs, bounds check missed_bytes: 10/10 replicas passed. Gate passed.
- quality_gate_metrics_logs, bounds check memory_usage: 10/10 replicas passed. Gate passed.
- quality_gate_metrics_logs, bounds check intake_connections: 10/10 replicas passed. Gate passed.
- quality_gate_metrics_logs, bounds check total_bytes_received: 10/10 replicas passed. Gate passed.
- quality_gate_idle_all_features, bounds check total_bytes_received: 10/10 replicas passed. Gate passed.
- quality_gate_idle_all_features, bounds check memory_usage: 10/10 replicas passed. Gate passed.
- quality_gate_idle_all_features, bounds check intake_connections: 10/10 replicas passed. Gate passed.
- quality_gate_idle, bounds check intake_connections: 10/10 replicas passed. Gate passed.
- quality_gate_idle, bounds check memory_usage: 10/10 replicas passed. Gate passed.
- quality_gate_idle, bounds check total_bytes_received: 10/10 replicas passed. Gate passed.
|
@codex review |
|
Gitlab CI Configuration ChangesModified Jobsbazel:test:windows-amd64 bazel:test:windows-amd64:
before_script:
- C:\ci-identities-gitlab-job-client.exe assume-role
id_tokens:
BUILDBARN_ID_TOKEN:
aud: buildbarn.us1.ddbuild.io
CI_IDENTITIES_GITLAB_ID_TOKEN:
aud: ci-identities
needs: []
rules:
- when: on_success
script:
- '$FailFast = @''
$ErrorActionPreference = ''Stop''
$PSNativeCommandUseErrorActionPreference = $true
Set-StrictMode -Version 3.0
''@
Invoke-Expression $FailFast
$Utf8Script = [Text.Encoding]::UTF8.GetString([Text.Encoding]::GetEncoding([Console]::OutputEncoding.CodePage).GetBytes($POWERSHELL_SCRIPT))
$EncodedCommand = [Convert]::ToBase64String([Text.Encoding]::Unicode.GetBytes("$FailFast`n$Utf8Script"))'
- .\tools\ci\docker-run-with-bazel-cache.ps1 --env=CI_IDENTITIES_GITLAB_ID_TOKEN
--volume="${CI_PROJECT_DIR}:$CI_PROJECT_DIR" --workdir="$CI_PROJECT_DIR" "$WINBUILDIMAGE"
powershell -NonInteractive -NoLogo -NoProfile -InputFormat Text -OutputFormat
Text -EncodedCommand $EncodedCommand
stage: source_test
tags:
- windows-v2:2022
timeout: 60m
variables:
ARCH: x64
AWS_SHARED_CREDENTIALS_FILE: ${CI_PROJECT_DIR}\.aws\credentials-by-job-id\${CI_JOB_ID}
BAZELISK_HOME: $XDG_CACHE_HOME/bazelisk
GIT_DEPTH: 2
OVERRIDE_GIT_STRATEGY: clone
- POWERSHELL_SCRIPT: bazel test --keep_going //... -- -//bazel/rules/dd_packaging/...
+ POWERSHELL_SCRIPT: bazel test --keep_going --verbose_failures //... -- -//bazel/rules/dd_packaging/...
? +++++++++++++++++++
-//packages/agent/linux/...
WINBUILDIMAGE: registry.ddbuild.io/ci/datadog-agent-buildimages/windows_ltsc2022_${ARCH}${CI_IMAGE_WIN_LTSC2022_X64_SUFFIX}:${CI_IMAGE_WIN_LTSC2022_X64}
XDG_CACHE_HOME: c:/bzlChanges Summary
ℹ️ Diff available in the job log. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 617cd8aaac
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| cc_import( | ||
| name = "_libsqlite3_shared", | ||
| shared_library = ":sqlite3", |
There was a problem hiding this comment.
libsqlite3_dynamic now depends on a cc_import that only sets shared_library; on Windows, cc_import requires an interface_library (.lib) to satisfy link-time resolution for DLL symbols. Because this target is injected into github.com/mattn/go-sqlite3 via cdeps, Windows builds that include sqlite3 (for example bazel test //... on Windows) can fail at link time when the import library is missing.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
libsqlite3_dynamic explicitly depend on :libsqlite3 on windows.
_libsqlite3_shared is added to the build graph on non-windows OSes only
|
This pull request has been automatically marked as stale because it has not had activity in the past 15 days. It will be closed in 30 days if no further activity occurs. If this pull request is still relevant, adding a comment or pushing new commits will keep it open. Also, you can always reopen the pull request if you missed the window. Thank you for your contributions! |
Introduce the building blocks required to redirect github.com/mattn/go-sqlite3 to the agent's own libsqlite3, without yet enabling the migration: - deps/sqlite3.BUILD.bazel: cherry-pick the feature defines that the binding applies in its bundled-amalgamation build (sqlite3.go:13-23) so the shared libsqlite3 is feature-equivalent. Add :libsqlite3_headers, :_libsqlite3_shared (cc_import of the cc_shared_library output), and :libsqlite3_dynamic, the cdeps-consumable wrapper. Pattern mirrors //rtloader:rtloader_dynamic. - bazel/rules/go_sqlite3/go_sqlite3_library.bzl: a go_library wrapper that enables cgo and injects @sqlite3//:libsqlite3_dynamic as a cdep, plus a TODO for the install-time rpath fix-up that will be needed once the agent binary is built with Bazel. - bazel/rules/go_sqlite3/BUILD.bazel: alias forwarding to @sqlite3//:libsqlite3_dynamic. @sqlite3 isn't visible from the bzlmod context of the gazelle-generated go-sqlite3 repo; the @@//... label resolves regardless and insulates the macro from canonical-name churn. No behavior change yet: these targets are inert until the gazelle override that emits go_sqlite3_library lands in a follow-up commit.
Stop compiling the bundled SQLite amalgamation that ships with github.com/mattn/go-sqlite3 and link the binding against //deps:sqlite3 instead. Removes one full SQLite C compile from every build, keeps a single .so in the runtime artifacts (already shared by cpython, rpm, openscap). - .bazelrc: enable the libsqlite3 Go build tag globally so sqlite3_libsqlite3.go compiles and defines USE_LIBSQLITE3, which makes the bundled sqlite3-binding.c a no-op TU and switches the cgo preamble to #include <sqlite3.h>. - deps/go.MODULE.bazel: add a gazelle_override on github.com/mattn/go-sqlite3 that (a) passes -build_tags=libsqlite3 so Gazelle picks the right sources in srcs, and (b) maps go_library to the go_sqlite3_library wrapper added in the previous commit, which injects cgo = True and the libsqlite3_dynamic cdep so #include <sqlite3.h> and -lsqlite3 resolve against //deps:sqlite3. Verified by building //pkg/security/resolvers/sbom/collectorv2:collectorv2_test and confirming via ldd that the binary dynamically links against the @sqlite3//:sqlite3 shared library (single agent-owned libsqlite3.so) and go-sqlite3's GoCompilePkg runs in ~0s (no amalgamation recompile).
617cd8a to
bb11d85
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bb11d85835
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| build_extra_args = ["-build_tags=libsqlite3"], | ||
| directives = [ | ||
| "gazelle:map_kind go_library go_sqlite3_library @@//bazel/rules/go_sqlite3:go_sqlite3_library.bzl", | ||
| ], | ||
| path = "github.com/mattn/go-sqlite3", |
There was a problem hiding this comment.
This new gazelle_override changes the go_deps module-extension inputs, but MODULE.bazel.lock is not part of the commit (git show --name-only 628a169d... only lists the .bazelrc, deps/go.MODULE.bazel, sqlite BUILD files, and task files). The Bazel review rules in bazel/codereview_guideline.md require committing the lockfile for MODULE/module-extension changes; otherwise CI/workflows that run with --lockfile_mode=error can fail as soon as Bazel re-evaluates the extension instead of updating the lockfile locally.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
The change is only about how gazelle will generate BUILD files relative to go-sqlite3, no change to the module graph
| if not glibc: | ||
| build_tags = list(set(build_tags).difference({"nvml"})) | ||
|
|
||
| build_tags = apply_sqlite3_build_flags(env, build_tags, embedded_path or get_embedded_path(ctx)) |
There was a problem hiding this comment.
When dda inv agent.build --static is used with the default Linux agent tags (trivy/podman), this call still adds the libsqlite3 build tag whenever an embedded libsqlite3.so is present. get_build_flags(static=True) has already forced external static linking, so go-sqlite3's -lsqlite3 will be resolved under -static while this helper only checked for the shared .so path, which can break static agent builds or make them depend on an unintended system archive. Pass the static mode through and leave go-sqlite3 on its bundled amalgamation for static builds unless a matching static archive is explicitly wired.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
There is no --static flag for the agent.build task
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d954b4c29d
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| go_deps.gazelle_override( | ||
| build_extra_args = ["-build_tags=libsqlite3"], |
There was a problem hiding this comment.
This adds a new go_deps.gazelle_override module-extension tag, but MODULE.bazel.lock is not part of the diff. The repo's Bazel guidelines require the lockfile to be committed for MODULE/module-extension changes, and CI invocations using --lockfile_mode=error will fail instead of regenerating it; please run bazel mod deps/bazel mod tidy and include the resulting lockfile update.
Useful? React with 👍 / 👎.
When a Bazel-installed embedded path contains libsqlite3.so + sqlite3.h (e.g. after `dda inv rtloader.build --enable-bazel`), set CGO_LDFLAGS, CGO_CFLAGS, and LD_LIBRARY_PATH to point at it, and inject the `libsqlite3` build tag so go-sqlite3 links against the shared library instead of recompiling its bundled amalgamation.
When a Bazel-installed embedded path contains libsqlite3.so + sqlite3.h (e.g. after `dda inv rtloader.build --enable-bazel`), set CGO_LDFLAGS, CGO_CFLAGS, and LD_LIBRARY_PATH to point at it, and inject the `libsqlite3` build tag so go-sqlite3 links against the shared library instead of recompiling its bundled amalgamation. Only activates when the build already contains a tag from SQLITE3_DEPENDENT_BUILD_TAGS (currently trivy/podman), so IoT agent, dogstatsd, and other builds that don't pull in go-sqlite3 are unaffected.
go-sqlite3's sqlite3_libsqlite3.go only adds '#cgo linux LDFLAGS: -lsqlite3'; on macOS it relies on the linker auto-linking system sqlite3 from SDK stubs. ld64.lld (LLVM's drop-in for Apple's ld64, used on CI) does not do that, so without an explicit -lsqlite3 the linker picks up Apple's system sqlite3 (which has SQLITE_OMIT_LOAD_EXTENSION) instead of ours, causing "undefined symbol: sqlite3_enable_load_extension" failures. Bazel: add -lsqlite3 to libsqlite3_dynamic's macOS linkopts; the -L search path for our library is already provided by the _libsqlite3_shared cc_import's CcInfo so this is sufficient to steer the linker to our build. dda-inv: explicitly append -lsqlite3 to CGO_LDFLAGS on darwin so the linker binds to our embedded libsqlite3.dylib rather than the system one.
The initial implementation used go-sqlite3's bundled-amalgamation compile flags (FTS3, R-tree, UPDATE/DELETE LIMIT, WAL synchronous, trace size limit) on the assumption that they were required for ABI compatibility. Auditing the actual go-sqlite3 consumers (MacPorts, Podman, go-rpmdb) shows none of them exercise those extensions; all queries are simple SELECTs against regular tables. The full feature set added ~879 KiB to the package unnecessarily. Keep only what the omnibus build already compiled with (MATH_FUNCTIONS, THREADSAFE=1) plus SQLITE_OMIT_DEPRECATED, which go-sqlite3 includes in its own bundled build so it never calls the removed APIs. The heroku / non-heroku select() is dropped as both flavors now share the same minimal set.
d954b4c to
021274d
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 021274d856
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| # go_sqlite3_library wrapper, which sets cgo = True and injects the | ||
| # @sqlite3//:libsqlite3_dynamic cdep so the binding's #include <sqlite3.h> and | ||
| # -lsqlite3 directives resolve. | ||
| go_deps.gazelle_override( |
There was a problem hiding this comment.
This adds a new go_deps.gazelle_override, which changes the module extension inputs, but the commit does not include a corresponding MODULE.bazel.lock update. The Bazel lint job I checked (.gitlab/build/bazel/lint.yml) runs bazel mod deps --lockfile_mode=refresh --repo_env=REPIN=1 followed by git diff --exit-code, so this will fail in CI whenever the refreshed lockfile records the new extension state. Please run the documented bazel mod deps --repo_env=REPIN=1 and commit the resulting lockfile changes.
Useful? React with 👍 / 👎.
|
This pull request has been automatically marked as stale because it has not had activity in the past 15 days. It will be closed in 30 days if no further activity occurs. If this pull request is still relevant, adding a comment or pushing new commits will keep it open. Also, you can always reopen the pull request if you missed the window. Thank you for your contributions! |
|
This pull request was automatically closed because it has been stale for 15 days with no activity. If this pull request is still relevant, please reopen it or create a new pull request with updated information. Thanks! |
1 participant
What does this PR do?
Stops compiling the SQLite amalgamation bundled with
github.com/mattn/go-sqlite3and links the binding against the agent's existing//deps:sqlite3shared library instead.Two commits:
go_sqlite3_libraryGazelle-emitted wrapper rule (viamap_kind), a:libsqlite3_dynamiccdeps target indeps/sqlite3.BUILD.bazel, and the feature defines that the binding applies in its default build (cherry-picked fromsqlite3.go:13-23). Inert on its own.libsqlite3Go build tag globally in.bazelrcand wire thegazelle_overridethat redirectsgo_libraryto the wrapper.Motivation
The agent already builds
libsqlite3.soonce (@sqlite3//:sqlite3, consumed by cpython, rpm, openscap). go-sqlite3 was independently rebuilding the same SQLite 3.53.0 amalgamation and statically linking it into every binary that imports the binding. Removes a duplicate native build and keeps a single.soin the runtime artifacts.Describe how you validated your changes
Local Linux:
bazel build //pkg/util/podman:podman— green; go-sqlite3GoCompilePkgruns in ~0s (bundled amalgamation is no-op'd viaUSE_LIBSQLITE3).bazel build //pkg/security/resolvers/sbom/collectorv2:collectorv2_test— green;lddconfirms the binary dynamically links against@sqlite3//:sqlite3'slibsqlite3.so.bazel run //:gazelleis idempotent across both commits.//pkg/inventory/software:softwareis darwin/ios-only and already wired in Bazel, so the macOS CI job will exercise it.Additional Notes
The wrapper macro carries a
TODO(agent-build)noting that rpath fix-up (same pattern cpython uses today viabazel/rules/rewrite_rpath) will be needed once the agent binary is built with Bazel — the current rpath points intobazel-bin/_solib_local/...and won't resolve in the install tree.