oracledbreceiver: optimize query sample collection by splitting into two-pass approach to avoid full cursor cache scan

[tharun0064]

Description

• Replaced the single-pass query that joined V$SESSION against V$SQL with a two-pass approach
• Pass-1 queries V$SESSION only, collecting active sessions with COALESCE(SQL_ID, PREV_SQL_ID) and COALESCE(SQL_CHILD_NUMBER, PREV_CHILD_NUMBER) as lookup keys
• Pass-2 fetches SQL_FULLTEXT, CHILD_ADDRESS, and PLAN_HASH_VALUE from V$SQL WHERE SQL_ID IN (...) using only the SQL IDs seen in active sessions
• Join between Pass-1 and Pass-2 results is done in Go, keyed by SQL_ID:CHILD_NUMBER
• Sessions whose cursor has aged out of the shared pool are skipped, preserving the previous inner-join semantics
• Added batching of the IN list in chunks of 1000 to avoid Oracle's ORA-01795 limit

Link to tracking issue

Fixes #49874

Testing

• Verified batching of IN list handles max_rows_per_query values above 1000 without ORA-01795
• Verified Pass-1 (session query) and Pass-2 (V$SQL lookup) execute correctly and produce matching query sample events
• Confirmed sessions with no matching cursor in V$SQL are dropped (preserving inner-join semantics from old single-pass query)

Documentation

Authorship

• I, a human, wrote this pull request description myself.

[opentelemetry-pr-dashboard]

Pull request dashboard status

Status last refreshed: 2026-07-27 15:34:29 UTC.

• Waiting on: Reviewers
• Next step: Review the latest changes.

This automated status or its linked feedback items may be incorrect. If something looks wrong, please report it with the result you expected.

[ebrdarSplunk]

Thanks for this — the two-pass rework is the right call. Dropping the V$SESSION↔V$SQL join means we stop full-scanning the shared-pool cursor cache and materializing SQL_FULLTEXT for every cursor, and the cost per scrape now tracks the number of active sessions instead of the cache size. That also makes it viable to lower the sample collection interval later, which is something we care about.

Before merge I'd like two changes:

1. IN-list batching binds the wrong placeholder numbers on the 2nd+ batch (receiver/oracledbreceiver/scraper.go, enrichSamplesWithSQLStats). The placeholder list is built once as :1..:N and then sliced per batch, so batch 2 emits :1001..:2000 while only 1000 args are passed to that statement. Details + suggested fix inline.

2. No test actually exercises the batching path. The current tests go through fakeDbClient, which ignores both the SQL text and the bind args, so the >1000 case is never validated (and couldn't detect point 1). Suggested test inline.

Minor, non-blocking:

• The changelog subtext is worth a sentence noting this changes a single read-consistent snapshot into two reads at slightly different SCNs. It's immaterial for actively-executing cursors (they're pinned and can't age out between the two passes), but it's a real semantic change worth documenting.
• The PR description says PREV_SQL_ID/PREV_CHILD_NUMBER fallback was "added" — that COALESCE fallback already existed in the old join and is carried over, so this isn't new behavior.

[ebrdarSplunk]

This reuses a global :1..:N placeholder list across batches, but each iteration is a separate statement whose args are bound at positions 1..len(batchIDs). Batch 1 is fine (:1..:1000); batch 2 slices out :1001..:2000 yet still passes only 1000 args, so those bind positions are never supplied. Depending on how go-ora maps numbered placeholders that's either a silent wrong-bind or ORA-01008: not all variables are bound.

Simplest robust fix is to regenerate :1..:n inside the batch loop so the numbers always match the args for that call (same convention as getChildAddressToPlanMap). Replace the id-collection + batch loop with something like:

    // Collect distinct, non-empty sql_ids in stable order.
    seen := make(map[string]struct{})
    var ids []any
    for _, row := range rows {
        id := row[lookupSQLID]
        if id == "" {
            continue
        }
        if _, dup := seen[id]; dup {
            continue
        }
        seen[id] = struct{}{}
        ids = append(ids, id)
    }
    if len(ids) == 0 {
        return nil
    }
    // Oracle caps an IN list at 1000 expressions; batch to stay under it.
    const oracleInLimit = 1000
    stats := make(map[string]metricRow, len(ids))
    for i := 0; i < len(ids); i += oracleInLimit {
        end := min(i+oracleInLimit, len(ids))
        batchIDs := ids[i:end]
        // Regenerate :1..:n for THIS batch so bind positions match the args
        // passed below, regardless of driver placeholder semantics.
        placeholders := make([]string, len(batchIDs))
        for j := range batchIDs {
            placeholders[j] = fmt.Sprintf(":%d", j+1)
        }
        sqlQuery := fmt.Sprintf(samplesStatsQuery, strings.Join(placeholders, ", "))
        statsRows, err := s.clientProviderFunc(s.db, sqlQuery, s.logger).metricRows(ctx, batchIDs...)
        if err != nil {
            return fmt.Errorf("failed to fetch V$SQL stats for query samples: %w", err)
        }
        for _, sr := range statsRows {
            stats[sr[sqlIDAttr]+":"+sr[childNumberAttr]] = sr
        }
    }

[tharun0064]

Thanks for catching this. Fixed in the latest commit

[ebrdarSplunk]

These tests route through fakeDbClient, whose metricRows(context.Context, ...any) discards both the SQL text and the bind args and returns canned rows. So none of the sample tests validate the batching in enrichSamplesWithSQLStats: every case uses ≤3 sql_ids (one batch), and even at >1000 the fake would return the same rows and hide a placeholder/arg mismatch.

Could you add a test that drives >1000 distinct sql_ids and asserts on the generated SQL + args? Something like:

    // capturingClient records each metricRows call so we can assert on the
    // generated SQL and bind args (fakeDbClient discards both).
    type capturingClient struct {
        sql    string
        calls  *[]capturedCall
        respFn func(args []any) []metricRow
    }
    type capturedCall struct {
        sql  string
        args []any
    }
    func (c *capturingClient) metricRows(_ context.Context, args ...any) ([]metricRow, error) {
        *c.calls = append(*c.calls, capturedCall{sql: c.sql, args: args})
        return c.respFn(args), nil
    }

    func TestEnrichSamplesBatchesInLists(t *testing.T) {
        const total = 2500 // -> 3 batches: 1000, 1000, 500
        sessionRows := make([]metricRow, total)
        statsBySQLID := make(map[string]metricRow, total)
        for i := 0; i < total; i++ {
            id := fmt.Sprintf("sqlid_%04d", i)
            sessionRows[i] = metricRow{"LOOKUP_SQL_ID": id, "LOOKUP_CHILD_NUMBER": "0"}
            statsBySQLID[id] = metricRow{
                "SQL_ID": id, "CHILD_NUMBER": "0", "CHILD_ADDRESS": "ADDR_" + id,
                "PLAN_HASH_VALUE": "1", "SQL_FULLTEXT": "text " + id,
            }
        }

        var calls []capturedCall
        scrpr := oracleScraper{
            logger:         zap.NewNop(),
            dbProviderFunc: func() (*sql.DB, error) { return nil, nil },
            clientProviderFunc: func(_ *sql.DB, q string, _ *zap.Logger) dbClient {
                return &capturingClient{sql: q, calls: &calls, respFn: func(args []any) []metricRow {
                    out := make([]metricRow, 0, len(args))
                    for _, a := range args {
                        if r, ok := statsBySQLID[a.(string)]; ok {
                            out = append(out, r)
                        }
                    }
                    return out
                }}
            },
        }

        require.NoError(t, scrpr.enrichSamplesWithSQLStats(t.Context(), sessionRows))

        require.Len(t, calls, 3)
        wantSizes := []int{1000, 1000, 500}
        for i, call := range calls {
            assert.Len(t, call.args, wantSizes[i])
            assert.Equal(t, wantSizes[i], strings.Count(call.sql, ":"),
                "placeholder count must equal arg count for batch %d", i)
            assert.Contains(t, call.sql, ":1,", "batch %d must start at :1", i)
            assert.Contains(t, call.sql, fmt.Sprintf(":%d)", wantSizes[i]),
                "batch %d must end at :%d", i, wantSizes[i])
        }
        for _, row := range sessionRows {
            assert.Equal(t, "text "+row["LOOKUP_SQL_ID"], row["SQL_FULLTEXT"])
        }
    }

On the current code this fails at batch 2 (:1, and :1000) are absent because it emits :1001..:2000); with the placeholder fix it passes and also proves the cross-batch join reassembles text/plan correctly.

[tharun0064]

Added a test case to cover this scenerio

[tharun0064]

Thanks for this — the two-pass rework is the right call. Dropping the V$SESSION↔V$SQL join means we stop full-scanning the shared-pool cursor cache and materializing SQL_FULLTEXT for every cursor, and the cost per scrape now tracks the number of active sessions instead of the cache size. That also makes it viable to lower the sample collection interval later, which is something we care about.

Before merge I'd like two changes:

1. IN-list batching binds the wrong placeholder numbers on the 2nd+ batch (receiver/oracledbreceiver/scraper.go, enrichSamplesWithSQLStats). The placeholder list is built once as :1..:N and then sliced per batch, so batch 2 emits :1001..:2000 while only 1000 args are passed to that statement. Details + suggested fix inline.
2. No test actually exercises the batching path. The current tests go through fakeDbClient, which ignores both the SQL text and the bind args, so the >1000 case is never validated (and couldn't detect point 1). Suggested test inline.

Minor, non-blocking:

• The changelog subtext is worth a sentence noting this changes a single read-consistent snapshot into two reads at slightly different SCNs. It's immaterial for actively-executing cursors (they're pinned and can't age out between the two passes), but it's a real semantic change worth documenting.
• The PR description says PREV_SQL_ID/PREV_CHILD_NUMBER fallback was "added" — that COALESCE fallback already existed in the old join and is carried over, so this isn't new behavior.

Thanks for catching this. Fixed in the latest commit — placeholders are now built once up to oracleInLimit and sliced per batch (maxPlaceholders[:len(batchIDs)]), so every batch always binds :1..:N. The test TestEnrichSamplesBatchesInLists (2500 rows → 3 batches of 1000/1000/500) validates the placeholder numbering and arg counts for each batch.

Updated the PR description and changelog

[ebrdarSplunk]

Great thank you, approved

[paulojmdias]

@open-telemetry/collector-contrib-approvers, missing your review. Approved by the codeowner already.