Skip to content

fix(burstiq): bound profiler memory, null-safe sample data, capped retry wait#30238

Open
akashverma0786 wants to merge 9 commits into
mainfrom
fix/burstiq-profiling-resilience
Open

fix(burstiq): bound profiler memory, null-safe sample data, capped retry wait#30238
akashverma0786 wants to merge 9 commits into
mainfrom
fix/burstiq-profiling-resilience

Conversation

@akashverma0786

@akashverma0786 akashverma0786 commented Jul 20, 2026

Copy link
Copy Markdown
Collaborator

FIX #30438

What

Two fixes to the BurstIQ connector's sampling / client path.

1. Null-safe sample data (no more "nan" strings)

BurstIQ omits absent fields per record, so reindexing pages to the column union leaves NaN/NaT gaps, and dropna(how="all") deliberately keeps partially-filled rows. Those gaps reached the sample-data sanitizer as non-finite floats and were serialized as the strings "nan" / "NaT" instead of JSON null. _fetch_rows now normalizes missing markers to None (guarded with is_scalar so list/dict cells are left alone). This affects sample-data display/upload only — profiler metrics use a separate path and are unchanged.

2. Cap the retry wait (no multi-hour hangs)

The client's urllib3 Retry used the default respect_retry_after_header=True with no cap (urllib3 2.6.1), so a 503 carrying a large Retry-After could make a single request sleep for the server-dictated duration (potentially hours), with no total-time budget. Setting respect_retry_after_header=False bounds the worst-case wait to our own backoff.

Testing

  • test_burstiq_sampler.py + test_burstiq_client.py36/36 pass, including 2 new:
    • test_missing_cells_become_none_not_nan — gaps serialize as null, not "nan"
    • test_session_retry_ignores_server_retry_afterrespect_retry_after_header is off
  • ruff check and ruff format --check clean.

Notes

  • No shared/base code touched — changes are confined to the BurstIQ connector, so no other connector is affected.

Summary by Gitar

  • Memory management:
    • Capped unbounded profiles at _MAX_PROFILE_ROWS (1M) to prevent potential OOM errors during ingestion.
    • Added warning logs when no profileSample is configured to notify users of memory bounding.
  • Null-safe sampling:
    • Normalized missing pandas markers to None using to_null to ensure JSON serialization handles them as null instead of string "nan".
  • Retry configuration:
    • Set respect_retry_after_header=False in BurstIQClient to prevent server-side Retry-After headers from causing multi-hour request hangs.
  • Testing:
    • Added unit test test_no_sample_caps_at_max_profile_rows to verify memory bounding behavior.

This will update automatically on new commits.

Greptile Summary

This PR improves resilience in the BurstIQ sampling and client paths. The main changes are:

  • Caps profiling reads when no sample limit is configured.
  • Converts missing pandas scalar values to JSON-compatible nulls.
  • Ignores server-provided Retry-After delays.
  • Adds focused tests for the new behavior.

Confidence Score: 5/5

This looks safe to merge.

  • The updated pagination path applies the default row cap.
  • Missing scalar values are converted without evaluating list or dictionary cells.
  • Retry delays remain controlled by the configured backoff.
  • No blocking issues were found in the changed code.

Important Files Changed

Filename Overview
ingestion/src/metadata/ingestion/source/database/burstiq/client.py Keeps retry delays under client control by ignoring server-provided Retry-After values.
ingestion/src/metadata/sampler/pandas/burstiq/sampler.py Bounds default profiling reads and converts missing scalar sample values to None.
ingestion/tests/unit/topology/database/test_burstiq_client.py Checks the configured retry count and Retry-After behavior.
ingestion/tests/unit/topology/database/test_burstiq_sampler.py Checks the default profile cap and missing-cell normalization.

Reviews (10): Last reviewed commit: "Merge branch 'main' into fix/burstiq-pro..." | Re-trigger Greptile

Context used (3)

  • Context used - openmetadata-ui-core-components/CLAUDE.md (source)
  • Context used - CLAUDE.md (source)
  • Context used - AGENTS.md (source)

@akashverma0786
akashverma0786 requested a review from a team as a code owner July 20, 2026 12:58
@github-actions

github-actions Bot commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

✅ PR checks passed

The linked issue has a description and all required Shipping project fields set. Thanks!

@github-actions

Copy link
Copy Markdown
Contributor

Hi there 👋 Thanks for your contribution!

The OpenMetadata team will review the PR shortly! Once it has been labeled as safe to test, the CI workflows
will start executing and we'll be able to make sure everything is working as expected.

Let us know if you need any help!

Comment thread ingestion/src/metadata/sampler/pandas/burstiq/sampler.py Outdated
- sampler: normalize NaN/NaT sample cells to None so uploads emit JSON null, not "nan"
- client: respect_retry_after_header=False so a 503 Retry-After can't hang a call for hours
- tests: null normalization, retry flag
@akashverma0786
akashverma0786 force-pushed the fix/burstiq-profiling-resilience branch from 3c364f6 to 5b330b9 Compare July 20, 2026 13:34
@github-actions

Copy link
Copy Markdown
Contributor

Hi there 👋 Thanks for your contribution!

The OpenMetadata team will review the PR shortly! Once it has been labeled as safe to test, the CI workflows
will start executing and we'll be able to make sure everything is working as expected.

Let us know if you need any help!

@akashverma0786 akashverma0786 added Ingestion safe to test Add this label to run secure Github workflows on PRs labels Jul 20, 2026
@github-actions

github-actions Bot commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

🔴 Playwright Results — workflow failed

Validated commit 6da5df4be20018d6ae5c6269461dd16deadd7a2b in Playwright run 30241289717, attempt 1.

✅ 105 passed · ❌ 2 failed · 🟡 0 flaky · ⏭️ 3 skipped · 🧰 0 lifecycle flaky

Performance

Blocking targets: ✅ met · Optimization targets: 🟡 in progress

Shard-job maxima below are not the full workflow wall time; the linked run includes build, fixture, planning, and reporting.

🕒 Full workflow signal wall (to summary) 45m 14s

⏱️ Max setup 3m 4s · max shard execution 12m 12s · max shard-job elapsed before upload 17m 17s · reporting 4s

🌐 205.01 requests/attempt · 1.74 app boots/UI scenario · 0.00% common-shard skew

Optimization targets still in progress:

  • Browser traffic was 205.01 requests per attempt (convergence target: fewer than 200).
  • Application boot ratio was 1.74 per UI scenario (210 boots / 121 scenarios; convergence target: at most 1).
Shard Passed Failed Flaky Skipped Lifecycle failed Lifecycle flaky
✅ Shard chromium-01 46 0 0 3 0 0
✅ Shard ingestion-01 28 0 0 0 0 0
🔴 Shard ingestion-02 31 2 0 0 0 0

Genuine Failures (failed on all attempts)

Features/TestSuiteMultiPipeline.spec.tsTestSuite multi pipeline support (shard ingestion-02)
Error: locator.click: Error: strict mode violation: getByText('Data Observability') resolved to 2 elements:     1) <div class="d-flex justify-between gap-1">Data Observability</div> aka getByTestId('profiler').getByText('Data Observability')     2) <span class="tw:truncate tw:text-xs tw:font-medium tw:text-primary">Data Observability</span> aka getByTestId('asset-health-row-dataObservability').getByText('Data Observability')  Call log: �[2m  - waiting for getByText('Data Observability')�[22m 
Features/TestSuiteMultiPipeline.spec.tsEdit the pipeline's test case (shard ingestion-02)
Error: locator.click: Error: strict mode violation: getByText('Data Observability') resolved to 2 elements:     1) <div class="d-flex justify-between gap-1">Data Observability</div> aka getByTestId('profiler').getByText('Data Observability')     2) <span class="tw:truncate tw:text-xs tw:font-medium tw:text-primary">Data Observability</span> aka getByTestId('asset-health-row-dataObservability').getByText('Data Observability')  Call log: �[2m  - waiting for getByText('Data Observability')�[22m 

📦 Download artifacts

How to debug locally
# Download playwright-test-results-<shard> artifact and unzip
npx playwright show-trace path/to/trace.zip    # view trace

Comment thread ingestion/src/metadata/sampler/pandas/burstiq/sampler.py
Reintroduces a row cap for the no-profileSample path so get_dataframes
can't materialize an unbounded BurstIQ chain into memory. Logs a warning
when the cap applies instead of silently truncating.
@gitar-bot

gitar-bot Bot commented Jul 27, 2026

Copy link
Copy Markdown
Code Review ✅ Approved 2 resolved / 2 findings

Bounds profiling memory at 1M rows and normalizes missing sample values to JSON-compatible nulls. Caps retry wait times by ignoring server-directed retry headers, resolving potential multi-hour request hangs.

✅ 2 resolved
Quality: Cap silently truncates profiling for chains over 1M rows

📄 ingestion/src/metadata/sampler/pandas/burstiq/sampler.py:87-98 📄 ingestion/src/metadata/sampler/pandas/burstiq/sampler.py:155-169
When an unsampled chain exceeds _MAX_PROFILE_ROWS (1,000,000), get_dataframes stops paginating at the cap and the profiler computes metrics on a truncated dataset with no log or warning, so operators can silently get profiles that don't reflect the full chain. Consider emitting a logger.warning when skip >= total_limit and more records likely remain (e.g. the last page was full), so the truncation is observable.

Performance: Removing row cap makes unsampled profiles load full chain into memory

📄 ingestion/src/metadata/sampler/pandas/burstiq/sampler.py:86-100 📄 ingestion/src/metadata/sampler/pandas/burstiq/sampler.py:154-168
With _MAX_PROFILE_ROWS deleted, _compute_total_limit returns None whenever no profileSample is set, so get_dataframes paginates the entire BurstIQ chain and holds every page in the frames list simultaneously (it is fully materialized before iter(frames) is returned). For large chains this is an unbounded in-memory accumulation that can OOM the ingestion worker — the opposite of the PR title's "bound profiler memory" goal. This fixes the prior 'silent truncation' concern but reintroduces the memory risk; consider retaining a (configurable/logged) upper bound, or emitting a warning when an unsampled chain exceeds a threshold, rather than removing the guard entirely.

Options

Display: compact → Showing less information.

Comment with these commands to change the behavior for this request:

Compact
gitar display:verbose         

Was this helpful? React with 👍 / 👎 | Gitar

@sonarqubecloud

Copy link
Copy Markdown

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Ingestion safe to test Add this label to run secure Github workflows on PRs To release Will cherry-pick this PR into the release branch

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Fix Burstiq p2 issue

1 participant