Skip to content

fix(data-insights): declare the data asset types once and make the field catalog deterministic - #31937

Closed
manerow wants to merge 4 commits into
mainfrom
fix/di-data-asset-types-single-declaration
Closed

fix(data-insights): declare the data asset types once and make the field catalog deterministic#31937
manerow wants to merge 4 commits into
mainfrom
fix/di-data-asset-types-single-declaration

Conversation

@manerow

@manerow manerow commented Aug 24, 2026

Copy link
Copy Markdown
Contributor

Fixes #31963

Three Data Insights bugs that share a shape: the chart returns a number that looks fine and is wrong. Nothing throws, nothing logs.

1. metric never reaches the chart builder

The set of entity types Data Insights covers was hand-written in three places:

Where Decides
A DataInsightSystemChartRepository.dataAssetTypes which indices the chart field catalog reads
B DataInsightsApp.dataAssetTypes which datastreams get created and written
C dataInsights/config.json keys the per-type attribute lists

B and C stay in sync only because a mismatch throws. Nothing keeps A honest.

#26260 added metric to B and C in March, to make metrics chartable. It was never added to A. So di-data-assets-metric has been created and filled daily ever since, while metricType, unitOfMeasurement and granularity have never appeared in the chart builder.

Fix: one enum, dataAssetType.json. A derives from it. B derives as "every type that no live index aliases in", which is the same 16 as before, so ingestion is unchanged.

B is deliberately not equal to A. The two data-quality types reach the wildcard through a dataInsightAliases entry pointing at the live index, so ingesting them would aim create/delete at live data.

2. The catalog changes on every server restart

getFieldNames deduplicated field names globally, so the first entity type iterated claimed a shared name permanently. The iteration source was a Set.of, whose order Java salts per JVM start.

Measured on a seeded instance: 277 records covering 14 of 17 types. topic shares all 28 of its field names with table, so it advertised zero fields despite holding 47,766 documents. Across the 68 possible orders, 9 outcomes are reachable and 17 types never is.

Someone picking "Topic" in the field picker got an empty list on roughly a coin flip.

Fix: dedupe by (entityType, name) instead of name. The type set also becomes a LinkedHashSet in enum order, so record order is stable too, which matters for any consumer that resolves a duplicated name by taking the first record.

3. Charts pick categories from data they don't count

A line chart with a terms x-axis picks its 100 buckets, then counts inside them with the metric filter applied. Selection ignored the filter.

So a "tables per service" chart ranked services by their total document count. A BI service with 5,000 dashboards and 3 tables outranked a warehouse service with 800 tables and nothing else. The first got a slot and rendered as 3. The second was missing from the chart entirely.

The axis selected its categories by a quantity the chart neither filters on nor displays. Measured on a seeded instance: a "Tier 1 assets per service" chart put test-dashboard-service (215,515 documents, zero Tier-1 assets) in the first slot, ahead of the one service that had any.

Fix: when every metric of a chart shares one filter, apply it to the request as well as inside the buckets. Within a bucket the filter is a conjunct applied twice, so the predicate is unchanged; what moves is which categories get a bucket at all.

Gated on the chart having a terms x-axis. A @timestamp chart has no top-N to align, and narrowing its query would shorten the window a date histogram plots, moving the first/last delta the dashboard renders. Six shipped charts have that shape.

4. An empty category takes the whole chart down with a 500

avg, min and max over a bucket the metric filter emptied return null, not 0 — there is no average of nothing. Both search clients hand that back as a boxed Double, and both aggregators unbox it straight into a primitive:

double value = aggregation.value();   // null -> NullPointerException

Nothing catches it, so the request dies and the user gets an HTTP 500 rather than a chart. sum and count return 0.0, which is why only those three functions are affected.

This is not new and it is not caused by this PR — reproduced on a main deployment with a three-line chart definition:

HTTP 500 - Cannot invoke "java.lang.Double.doubleValue()" because the return value of
"...SingleMetricAggregateBase.value()" is null

It is fixed here because §3 makes it reachable again. Ranking by the measure deliberately keeps the categories the filter matched nothing in — exactly the buckets that return null. Hoisting had been hiding the bug by deleting them first.

Fix: null-check before unboxing, on both engines, alongside the NaN/Infinity checks already there. A category with no value is skipped rather than emitted as 0, since 0 is a legitimate average and would be a wrong answer.

OpenSearch routes every metric type through one method, so guarding it covers all of them. Elasticsearch split the same job across three overloads and guarding only one left an asymmetry, so the redundant ValueCountAggregate overload is gone — it extends SingleMetricAggregateBase, so value_count now binds to the guarded path like everything else. No behaviour change: value_count never returns null.

Worth noting for reviewers: this is the only change here that fixes a live 500 on main, and it has no dependency on the rest of the PR.

Also fixed in the same files

  • getEntityAttributeFields returned the shared common list and appended to it in place. The workflow parses the config once and loops over every type, so each type inherited the attributes of every type before it, and which ones followed the salted order above.
  • The same method now fails with a readable message when a type has no mappingFields entry, instead of an NPE.
  • mappingFields keys are typed by the enum, so a misspelling fails at load rather than silently producing documents without their type-specific attributes.

What changes for consumers

The catalog grows from ~277 records to ~1,082. This is union versus sum: id exists on every type and was listed once, now once per type. No new information, correct attribution.

Consumers must group by entityType. Collate's drag-and-drop asset panel already does, and this fixes it there (topic goes from 0 fields to 52). Its three flat-list pickers do not, and are handled in collate#6077.

Filtered category charts stop drawing zero bars, because terms uses min_doc_count: 1. The clearest case is a service Insights tab: healthy_data_assets is requested with a service filter and today returns ~100 rows of which 99 are zero, so the widget reports another service's number over a 99 "day" window. It now returns the one service.

Counts can move up. Terms samples per shard, and a category outside a shard's cutoff loses that shard's contribution. Re-ranking over the filtered population promotes exactly those marginal categories, so one can come back with a larger, more complete number. Counts move toward completeness; they are not asserted unchanged.

This is the pre-existing terms approximation getting smaller, not a new source of error. Elasticsearch's condition for an accurate doc_count and accurate sub-aggregations is that the population a bucket is ranked on equals the population it counts. Before this change those two disagree by construction -- ranked on unfiltered documents, counted on filtered ones -- which is the regime in which shard truncation does the most damage. Hoisting makes them the same population again.

What this does not fix

  • size(100) is unchanged. This changes which hundred categories you get, not how many.
  • Selection aligns to the filter but not to the measure: a sum(k='size') chart still ranks by document count.
  • The groupBy dimension keeps the same misalignment: it builds its own terms(...).size(100) and is untouched whenever the x-axis is @timestamp.
  • A formula's q='...' is deliberately not hoisted. Doing so would scope the denominator of every percentage chart and make them all read 100%.
  • Charts whose metrics carry different filters, or where only some are filtered, are skipped.

Tests

Each verified to fail against the bug it guards:

Revert Fails with
metric removed from config.json coverage test names metric
dedup back to global expected: <[table, topic]> but was: <[table]>
type set back to toUnmodifiableSet() three runs, three different orders, all caught
common copy removed expected: <[id, service]> but was: <[id, columns, service]>
hoist removed 2 failures per engine
gate widened to groupBy onlyATermsAxisIsHoistable
null guard removed the exact Double.doubleValue() NPE, per engine

The 12 pre-existing aggregator tests pass unmodified. None of their fixtures sets a metric filter, which is what proves an unfiltered chart still produces a byte-identical request.

Greptile Summary

The PR centralizes the Data Insights asset-type contract and makes field-catalog generation deterministic. It also aligns categorical bucket selection with metric filters and safely skips null aggregate values.

  • Derives catalog and ingestion type sets from the shared schema enum while excluding live-index aliases from ingestion.
  • Deduplicates catalog fields by entity type and field name and prevents shared configuration-list mutation.
  • Orders categorical terms buckets by filtered participation across Elasticsearch and OpenSearch.
  • Adds focused unit and real-engine coverage for configuration, catalog, filtering, ordering, and null aggregates.

Confidence Score: 5/5

The PR appears safe to merge.

No blocking failure remains.

Important Files Changed

Filename Overview
openmetadata-service/src/main/java/org/openmetadata/service/apps/bundles/insights/DataInsightsApp.java Derives the stable ingestion set from the shared enum while excluding types supplied through live-index aliases.
openmetadata-service/src/main/java/org/openmetadata/service/apps/bundles/insights/search/DataInsightsSearchConfiguration.java Replaces untyped mapping-field keys with enum-backed configuration and reports invalid keys during loading.
openmetadata-service/src/main/java/org/openmetadata/service/apps/bundles/insights/search/DataInsightsSearchInterface.java Copies common fields before appending type-specific attributes and reports missing type mappings explicitly.
openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/DataInsightSystemChartRepository.java Uses enum declaration order for deterministic and complete Data Insights field-catalog enumeration.
openmetadata-service/src/main/java/org/openmetadata/service/search/DataInsightMetricFilter.java Centralizes extraction of metric-filter query JSON for both search engines.
openmetadata-service/src/main/java/org/openmetadata/service/search/elasticsearch/dataInsightAggregators/ElasticSearchLineChartAggregator.java Orders categorical buckets through the metric filter aggregation while preserving date-histogram behavior.
openmetadata-service/src/main/java/org/openmetadata/service/search/opensearch/dataInsightAggregator/OpenSearchLineChartAggregator.java Implements the OpenSearch counterpart of filter-aligned categorical bucket ordering.
openmetadata-spec/src/main/resources/json/schema/dataInsight/custom/dataAssetType.json Establishes the schema-backed source of truth for Data Insights asset types.
openmetadata-ui/src/main/resources/ui/src/generated/dataInsight/custom/dataAssetType.ts Keeps the generated TypeScript enum aligned with the shared schema contract.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart LR
  Schema[DataAssetType schema enum] --> Catalog[Chart field catalog types]
  Schema --> Ingestion[Ingested asset types]
  Mapping[Index mappings and live aliases] --> Ingestion
  Config[Per-type mapping fields] --> Streams[Data Insights documents]
  Ingestion --> Streams
  Streams --> Terms[Categorical terms axis]
  Filter[Metric filter] --> Wrapper[Filtered metric aggregation]
  Wrapper --> Terms
  Terms --> Results[Chart results]
  Results --> NullGuard[Skip null aggregate values]
Loading

Reviews (23): Last reviewed commit: "fix(data-insights): rank a categorical a..." | Re-trigger Greptile

Context used (3)

@manerow
manerow requested a review from a team as a code owner August 24, 2026 07:07
@github-actions

github-actions Bot commented Aug 24, 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 github-actions Bot added backend safe to test Add this label to run secure Github workflows on PRs labels Aug 24, 2026
@github-actions

Copy link
Copy Markdown
Contributor

✅ TypeScript Types Auto-Updated

The generated TypeScript types have been automatically updated based on JSON schema changes in this PR.

@github-actions

github-actions Bot commented Aug 24, 2026

Copy link
Copy Markdown
Contributor

Jest test Coverage

UI tests summary

Lines Statements Branches Functions
Coverage: 67%
67.08% (80471/119945) 51.47% (49274/95720) 52.45% (14716/28053)

@github-actions

github-actions Bot commented Aug 24, 2026

Copy link
Copy Markdown
Contributor

✅ Playwright Results — workflow succeeded

Validated commit 6ba2b6b3e21286a745488923aba98d89fe377f8d in Playwright run 32957616628, attempt 1.

✅ 1280 passed · ❌ 0 failed · 🟡 3 flaky · ⏭️ 0 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) 51m 15s

⏱️ Max setup 4m 24s · max shard execution 17m 50s · max shard-job elapsed before upload 21m 17s · reporting 8s

🌐 197.86 requests/attempt · 2.13 app boots/UI scenario · 29.44% common-shard skew

Optimization targets still in progress:

  • Common shard skew was 29.44% (convergence target: at most 15%).
  • Application boot ratio was 2.13 per UI scenario (2790 boots / 1308 scenarios; convergence target: at most 1).
Shard Passed Failed Flaky Skipped Lifecycle failed Lifecycle flaky
✅ Shard chromium-01 146 0 0 0 0 0
✅ Shard chromium-02 159 0 0 0 0 0
✅ Shard chromium-03 154 0 0 0 0 0
✅ Shard chromium-04 163 0 0 0 0 0
🟡 Shard chromium-05 152 0 1 0 0 0
✅ Shard chromium-06 155 0 0 0 0 0
🟡 Shard chromium-07 162 0 2 0 0 0
✅ Shard data-asset-rules-01 61 0 0 0 0 0
✅ Shard domain-isolation-01 16 0 0 0 0 0
✅ Shard global-state-01 34 0 0 0 0 0
✅ Shard import-export-01 7 0 0 0 0 0
✅ Shard ingestion-01 2 0 0 0 0 0
✅ Shard reindex-01 28 0 0 0 0 0
✅ Shard search-01 12 0 0 0 0 0
✅ Shard search-rbac-01 29 0 0 0 0 0
🟡 3 flaky test(s) (passed on retry)
  • Features/Table.spec.tsshould persist page size (shard chromium-05, 1 retry)
  • Features/AdvancedSearch.spec.tsColumn Tags Any in [tag1, tag2] returns both tables (shard chromium-07, 1 retry)
  • Pages/ExplorePageRightPanel_KnowledgeCenter.spec.tsShould remove user owner for knowledgeCenter (shard chromium-07, 1 retry)

📦 Download artifacts

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

@manerow manerow self-assigned this Aug 25, 2026
@gitar-bot

gitar-bot Bot commented Aug 26, 2026

Copy link
Copy Markdown
Code Review 👍 Approved with suggestions 4 resolved / 5 findings

Centralizes Data Insights asset types, makes field-catalog generation deterministic, and aligns filtered terms-axis category selection with the metric population. Consider addressing the minor formula population ranking observation.

💡 Edge Case: All-narrowed formula ranks axis by one operand's population

📄 openmetadata-service/src/main/java/org/openmetadata/service/search/elasticsearch/dataInsightAggregators/ElasticSearchDynamicChartAggregatorInterface.java:118-132 📄 openmetadata-service/src/main/java/org/openmetadata/service/search/opensearch/dataInsightAggregator/OpenSearchDynamicChartAggregatorInterface.java:113-127

In getDateHistogramByFormula, when a metric has a filter and every formula term also carries its own q= (all wrappers land in narrowed, unnarrowed stays empty), the method returns the narrowed keys, so the terms axis ranks by filter0 = (metricFilter AND numerator's q=). That is exactly the 'categories satisfying one operand' selection the javadoc says it avoids by leading with an unnarrowed wrapper. It only self-corrects when at least one term lacks a q=. For a ratio like count(q='a')/count(q='b') under a metric filter, categories that have denominator data but an empty numerator can be dropped from the top-N. Low impact and rare, but consider ranking by the metric filter itself (build an unnarrowed wrapper unconditionally when filter != null) so selection reflects the population the formula is evaluated over.

✅ 4 resolved
Performance: Per-type dedup scans the whole growing catalog (O(n^2))

📄 openmetadata-service/src/main/java/org/openmetadata/service/search/elasticsearch/ElasticSearchDataInsightAggregatorManager.java:233-244 📄 openmetadata-service/src/main/java/org/openmetadata/service/search/opensearch/OpenSearchDataInsightAggregatorManager.java:236-244
getFieldNames deduplicates via fieldList.stream().noneMatch(...), scanning the entire accumulated list on every leaf field across all entity types. Since this PR grows the catalog from ~277 to ~1000 records, the number of comparisons grows roughly quadratically (~13x). It remains sub-millisecond at current sizes so this is minor, but the scan could be restricted to the current entity type (or backed by a Set<String> of seen entityType+name keys) to keep it linear as coverage expands.

Quality: Test mutates global static SearchRepository, leaking across JVM fork

📄 openmetadata-service/src/test/java/org/openmetadata/service/apps/bundles/insights/DataAssetTypeCoverageTest.java:42-51
giveTheRepositoryASearchClient calls Entity.setSearchRepository(mock) on the static field Entity.searchRepository (Entity.java:110), which persists for the rest of the surefire JVM fork. Because it only sets when currently null, a later-loaded test that expects to configure its own repository — or that runs after this one and relies on a real/absent instance — can silently observe this mock, making failures order-dependent. Consider resetting the prior value in an @AfterAll, or loading the class reflectively without touching the global singleton.

Bug: Terms order path with dotted field name may fail server-side

📄 openmetadata-service/src/main/java/org/openmetadata/service/search/DataInsightMetricFilter.java:92-94 📄 openmetadata-service/src/main/java/org/openmetadata/service/search/elasticsearch/dataInsightAggregators/ElasticSearchLineChartAggregator.java:99-113 📄 openmetadata-service/src/main/java/org/openmetadata/service/search/elasticsearch/dataInsightAggregators/ElasticSearchLineChartAggregator.java:148-162 📄 openmetadata-service/src/main/java/org/openmetadata/service/search/opensearch/dataInsightAggregator/OpenSearchLineChartAggregator.java:94-108 📄 openmetadata-service/src/main/java/org/openmetadata/service/search/opensearch/dataInsightAggregator/OpenSearchLineChartAggregator.java:145-159
measureOrderPath() builds the terms order bucket path as filter> + metric.getField() + 0, e.g. filter>id.keyword0, and the sub-aggregation is genuinely named id.keyword0 (field+"0"). Elasticsearch/OpenSearch AggregationPath parsing splits the final path element on . into an aggregation name and a metric key, so filter>id.keyword0 is read as aggregation id with metric key keyword0, which does not exist and raises Invalid aggregation order path ... Unknown aggregation [id]. Since virtually every Data Insight metric ranks a keyword field (id.keyword, service.name.keyword), this path would throw at query time for the common case, turning a wrong-but-rendered chart into a failing query. The new tests only assert JSON serialization, not that a cluster accepts the path, so they do not catch this. Please verify against a live cluster; if confirmed, order by a dot-free sub-aggregation name (or wrap the value in an alias whose name has no dots) so the order path resolves.

Quality: Inlined terms-axis builder duplicates include/exclude logic 4x

📄 openmetadata-service/src/main/java/org/openmetadata/service/search/elasticsearch/dataInsightAggregators/ElasticSearchLineChartAggregator.java:71-85 📄 openmetadata-service/src/main/java/org/openmetadata/service/search/elasticsearch/dataInsightAggregators/ElasticSearchLineChartAggregator.java:129-143 📄 openmetadata-service/src/main/java/org/openmetadata/service/search/opensearch/dataInsightAggregator/OpenSearchLineChartAggregator.java:70-84 📄 openmetadata-service/src/main/java/org/openmetadata/service/search/opensearch/dataInsightAggregator/OpenSearchLineChartAggregator.java:128-142
Deleting the shared termsAxis helper re-duplicates the include/exclude terms-construction logic four times (initial build + sub-aggregation rebuild, across both ElasticSearch and OpenSearch aggregators), including the awkward pattern of calling a.terms(...) two or three times just to conditionally add include/exclude. It is functionally correct and mirrors the existing groupBy pattern, but the deleted helper centralized this in one place; the inlined form is harder to keep in sync between the two engines. Consider restoring a small shared builder helper (without the now-removed order parameter) to keep the include/exclude construction in one spot.

🤖 Prompt for agents
Code Review: Centralizes Data Insights asset types, makes field-catalog generation deterministic, and aligns filtered terms-axis category selection with the metric population. Consider addressing the minor formula population ranking observation.

1. 💡 Edge Case: All-narrowed formula ranks axis by one operand's population
   Files: openmetadata-service/src/main/java/org/openmetadata/service/search/elasticsearch/dataInsightAggregators/ElasticSearchDynamicChartAggregatorInterface.java:118-132, openmetadata-service/src/main/java/org/openmetadata/service/search/opensearch/dataInsightAggregator/OpenSearchDynamicChartAggregatorInterface.java:113-127

   In getDateHistogramByFormula, when a metric has a filter and every formula term also carries its own q= (all wrappers land in `narrowed`, `unnarrowed` stays empty), the method returns the narrowed keys, so the terms axis ranks by `filter0` = (metricFilter AND numerator's q=). That is exactly the 'categories satisfying one operand' selection the javadoc says it avoids by leading with an unnarrowed wrapper. It only self-corrects when at least one term lacks a q=. For a ratio like `count(q='a')/count(q='b')` under a metric filter, categories that have denominator data but an empty numerator can be dropped from the top-N. Low impact and rare, but consider ranking by the metric filter itself (build an unnarrowed wrapper unconditionally when filter != null) so selection reflects the population the formula is evaluated over.

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 👍 / 👎 | Powered by Gitar — free for open source

@manerow

manerow commented Aug 26, 2026

Copy link
Copy Markdown
Contributor Author

Superseded by a fresh PR from the same branch: the history here contained a request-narrowing approach that was later replaced by axis ranking, and the timeline no longer matches the code. Reopening clean.

@manerow

manerow commented Aug 26, 2026

Copy link
Copy Markdown
Contributor Author

Continued in #32115 (same branch, rebuilt history).

@sonarqubecloud

Copy link
Copy Markdown

@sonarqubecloud

Copy link
Copy Markdown

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

Labels

backend safe to test Add this label to run secure Github workflows on PRs

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Data Insights /charts/fields omits metric and changes its contents on every restart

1 participant