feat(contrib): add integration for aerospike/aerospike-client-go.v7#4830
Merged
Conversation
Contributor
🎉 All green!🧪 All tests passed 🎯 Code Coverage (details) 🔗 Commit SHA: 7753488 | Docs | Datadog PR Page | Give us feedback! |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files
🚀 New features to boost your workflow:
|
darccio
force-pushed
the
dario.castane/dsr/aerospike
branch
from
June 1, 2026 13:44
78b032a to
a1056ac
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
BenchmarksBenchmark execution time: 2026-07-01 11:10:52 Comparing candidate commit 7753488 in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 324 metrics, 2 unstable metrics, 1 known flaky benchmarks.
|
…w wrap-expression type change
kakkoyun
reviewed
Jun 1, 2026
kakkoyun
left a comment
kakkoyun
left a comment
Member
There was a problem hiding this comment.
LGTM.
govulncheck seems unhappy though.
darccio
added
the
AI Generated
darccio
force-pushed
the
dario.castane/dsr/aerospike
branch
from
June 1, 2026 21:11
5f3239c to
f33851b
Compare
darccio
marked this pull request as ready for review
June 2, 2026 10:02
… to avoid re-weaving the client library The struct-definition aspect injected WithContext/__ddGetCtx into aerospike-client-go/v7.Client, forcing Orchestrion to re-weave the entire (~150-file) aerospike package and invalidate its compile cache (the slow "hang" on PR #4830). It existed only to parent spans created in goroutines that capture an outer context. That mechanism could not have worked anyway: Orchestrion's GLS is goroutine- local (stored on runtime.g, cleared at goexit1) and is never copied across goroutine boundaries. The supported way to parent across a goroutine is to pass the context in as a parameter so the method-call aspect resolves it via .Function.ArgumentOfType. - Remove the Client.WithContext struct-definition aspect; the method-call aspect (wrap-expression on call sites) no longer touches the library. - Revert the else branch to WrapClientWithContext(x, nil) (GLS fallback), which also fixes the receiver double-evaluation in {{ .AST.Fun.X }}.__ddGetCtx(). - Rewrite TestCaseConcurrent to pass ctx into each goroutine as a parameter instead of asserting an injected WithContext method.
…pect for goroutine span propagation Re-introduces the Client.WithContext struct-definition aspect that was dropped in the previous commit. The struct-definition aspect injects WithContext/__ddGetCtx into as.Client so that users can propagate the active span into goroutines via client.WithContext(ctx).Method(...), without needing to pass ctx as a goroutine parameter. The method-call aspect's else-branch is restored to call __ddGetCtx() so that the goroutine-local context set by WithContext is picked up at the call site when no context.Context argument is in scope. The integration test keeps the ctx-as-parameter pattern introduced earlier; both propagation patterns are valid and documented.
…or method instrumentation Replace the struct-definition (WithContext/__ddGetCtx) + method-call approach with a function-body approach that instruments each *as.Client method directly inside the aerospike library. Two aspects: - Client (struct-definition): injects __dd_startAerospikeSpan and __dd_finishAerospikeSpan helpers into the aerospike package. - Client.Operation.Error / Client.Operation.ValueError (function-body): prepend span start/finish to each method body, dispatching on whether the method returns (error) or (value, error). Span parenting relies on the tracer's GLS (same goroutine). Spans started inside goroutines are roots; TestCaseConcurrent.ExpectedTraces is updated to reflect this. Also adds tracing.StartDefaultSpan for the Orchestrion advice template.
Member
Author
|
@rarguelloF The |
Member
Author
|
@codex review |
![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: 6b1bba50c6
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…st for unambiguous span matching Replace three identical Put goroutines with Put/Get/Delete, one per goroutine, each operating on its own pre-populated key. The three expected root spans now have distinct resource tags so each matches a different actual span — no two expected entries collapse onto the same received span. getKey and delKey are written in Setup (before the tracer starts) to avoid spurious Put spans in the expected trace output.
… parenting
The function-body aspect relies on tracer GLS, which is goroutine-local
and not copied across goroutine boundaries. To parent spans in goroutines,
a WithContext method is injected into *as.Client via struct-definition:
go func() { client.WithContext(ctx).Put(nil, key, bins) }()
WithContext stores ctx in a sync.Map keyed by goroutine ID.
__dd_aerospike_get_ctx (called by __dd_startAerospikeSpan) retrieves and
removes it so the immediately following method call picks up the right
parent. When WithContext was not used, it falls back to context.Background()
and the tracer's GLS provides same-goroutine parenting automatically.
TestCaseConcurrent is updated to exercise this path: each goroutine calls
applyCtx(client, ctx) (interface-asserted so the file compiles without
Orchestrion) before a distinct operation (Put/Get/Delete), and the expected
traces assert all three as children of test.root with unique resource names.
WrapClient's embedded *as.Client promoted PutObject, GetObject, BatchGetObjects, ScanAllObjects, ScanNodeObjects, ScanPartitionObjects, QueryObjects, QueryNodeObjects, QueryPartitionObjects, and QueryAggregate to the raw untraced client. Add explicit wrapper methods for all ten so every call site produces a span regardless of which API style is used. Add the same ten methods to the Orchestrion function-body aspects so automatic instrumentation covers them too: - PutObject / GetObject → Client.Operation.Error group (single Error return) - remaining eight → Client.Operation.ValueError group ((value, Error) return) Add one integration test per method following the existing TestPut/TestGet pattern. Object-API tests use a shared testRecord struct with as: field tags; scan/query tests drain the result channel and close the Recordset before asserting on spans.
Member
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4bf81ecb25
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
rachelyangdog
approved these changes
Jun 30, 2026
…e dead code The aerospike-client-go/v7 library gates its reflection-based Object API (PutObject, GetObject, BatchGetObjects, Scan*Objects, Query*Objects) behind //go:build !as_performance in client_reflect.go. This contrib was calling those methods unconditionally, so any application building with -tags as_performance would fail to compile. Move the nine Object-API wrappers into aerospike_object.go with a matching //go:build !as_performance constraint. QueryAggregate is not part of the Object API and stays in aerospike.go. Move the corresponding integration tests and testRecord into aerospike_object_test.go (no build tag — tests are never built with as_performance). Add WithService assertions to the Object tests to match the consistency of the data-API tests. Also delete the dead WrapClientWithContext / orchestrionCfg code left over from the abandoned wrap-expression join-point approach — nothing in the contrib or _integration tree calls either function. Remove the now-unused sync import. Add comments to orchestrion.yml noting: (1) the Object-method join points are harmless no-ops under as_performance (unmatched join points are silently skipped); (2) the goroutine-exit hazard in the __dd_aerospike_ctxs sync.Map (entries stored by WithContext are consumed by LoadAndDelete at the next instrumented call; if the goroutine exits first the entry persists until the runtime recycles its ID).
Without an entry in contribIntegrations, instrumentation.Load →
tracer.MarkIntegrationImported("github.com/aerospike/aerospike-client-go/v7")
returns false and Aerospike never appears in startup integration telemetry.
The key must match TracedPackage from instrumentation/packages.go.
docker-compose.yaml already pinned aerospike:ce-7.2.0.6 with a full SHA digest. Align ci-services.json and StartAerospikeTestContainer to use the same digest, preventing a silent test-environment change if the tag is ever repointed.
…e in Setup The default NewExponentialBackOff has a 15-minute MaxElapsedTime, meaning Setup hangs for up to 15 minutes if the Aerospike container never becomes healthy. Cap it at 2 minutes to fail fast on broken environments.
Member
Author
|
@codex review |
Contributor
Config Audit |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 35a8fbe7b8
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
… pairs
In aerospike-client-go/v7, four public methods are 1-line thin wrappers that
immediately delegate to another public method also in the function-body advice
list:
Query → QueryPartitions
ScanAll → ScanPartitions
QueryObjects → QueryPartitionObjects (!as_performance only)
ScanAllObjects → ScanPartitionObjects (!as_performance only)
Without a guard, one user-facing call produces two spans (one per aspect fire).
Node variants (ScanNode, QueryNode, …) delegate to unexported methods and are
unaffected.
Fix: embed an unexported __dd_aerospike_active_key sentinel in the goroutine's
context when an instrumented call starts. Rewrite both prepend-statements
templates to:
1. Pop the context via __dd_aerospike_get_ctx() explicitly.
2. If the sentinel is absent (outermost call): store a marked context back in
__dd_aerospike_ctxs so the inner delegated call sees it, create a span,
and defer deletion + span finish.
3. If the sentinel is present (nested/delegated call): store the context back
for further nesting, skip span creation.
No new data structure is introduced; the existing __dd_aerospike_ctxs sync.Map
carries the marker. Direct calls to QueryPartitions/ScanPartitions continue to
produce one correctly-named span. __dd_startAerospikeSpan is removed (the
templates now call tracing.StartDefaultSpan directly to avoid double-popping the
context).
Add unit tests TestScanAll, TestScanPartitions, TestQuery, TestQueryPartitions
to aerospike_test.go. Each uses require.Len(t, spans, 1) to assert exactly one
span per call, locking down the 1-span contract for the delegation pairs in the
manual-wrapper path.
…ases Add TestCaseScanAll and TestCaseQuery to the Orchestrion integration suite. Each verifies that the expected span (resource "ScanAll" / "Query") is produced as a child of test.root when ScanAll or Query is called on a raw *as.Client. These cases document the expected single-span behaviour for the delegation pairs and would catch regressions where spans go missing. Note: the harness uses partial child-matching, so extra (double) spans do not automatically fail the test; exact-count enforcement for the Orchestrion path requires INTEGRATION=1 and a live server.
… advices Each prepend-statements advice block needs its own imports section for external package identifiers used in the template. The inject-declarations imports only apply to the generated file for the struct injection, not to the files where function bodies are instrumented.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Updates #1036 so it uses the current contrib instrumentation package, implementing it for the major version
v7.Latest major version
v8will be done in an upcoming PR.Motivation
Add support for Aerospike v7.
Reviewer's Checklist
make lintlocally.make testlocally.make generatelocally.make fix-moduleslocally.Unsure? Have a question? Request a review!