Fixes: latestKpiResult returns most recent bucket, not the partial leading one#30166
Open
ulixius9 wants to merge 1 commit into
Open
Fixes: latestKpiResult returns most recent bucket, not the partial leading one#30166ulixius9 wants to merge 1 commit into
ulixius9 wants to merge 1 commit into
Conversation
… leading one getKpiResult() and fetchAndSetKpiResults() computed the "latest" KPI value by aggregating a trailing 24h window and taking results.get(0). That window straddles two day buckets and the first (oldest) bucket is a partial fragment that aggregates to 0.0, so /latestKpiResult reported 0.0 while the day-aligned /kpiResult series correctly showed the real value. Select the most recent bucket (getLast()) via a shared getMostRecentResult() helper instead. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
❌ PR checklist incompleteThis PR cannot be merged until the following are addressed on its linked issue:
The fields live on the linked issue in the Shipping project (open the issue → right sidebar → Projects). After you set them, re-run this check (or push a commit) — issue/project changes do not re-trigger it automatically. Maintainers can bypass this check by adding the |
github-actions
Bot
added
Ingestion
safe to test
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Code Review ✅ ApprovedUpdates OptionsDisplay: compact → Showing less information. Comment with these commands to change the behavior for this request:
Was this helpful? React with 👍 / 👎 | Gitar |
|
Contributor
🟡 Playwright Results — all passed (27 flaky)✅ 4542 passed · ❌ 0 failed · 🟡 27 flaky · ⏭️ 95 skipped
🟡 27 flaky test(s) (passed on retry)
How to debug locally# Download playwright-test-results-<shard> artifact and unzip
npx playwright show-trace path/to/trace.zip # view trace |
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
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.
Describe your changes:
Fixes #
I fixed
/api/v1/kpi/{fqn}/latestKpiResultreturning0.0while the day-aligned/kpiResulttime series correctly showed the real value (e.g.0.68).KpiRepository.getKpiResult()(and the bulkfetchAndSetKpiResults()path) aggregated a trailing 24h window and tookresults.get(0)— but that window straddles two day buckets and the first/oldest one is a partial fragment that aggregates to0.0. I introduced a sharedgetMostRecentResult()helper that selects the most recent bucket viagetLast()and routed both call sites through it.Type of change:
High-level design:
N/A — small change.
Tests:
Use cases covered
latestKpiResultfor a KPI now returns the most recent day's value instead of the partial leading bucket's0.0.Unit tests
openmetadata-service/src/test/java/org/openmetadata/service/jdbi3/KpiRepositoryTest.javalatestResultUsesMostRecentBucketNotFirst— asserts the newest bucket (0.68) is chosen over the leading0.0bucket (fails against the oldget(0)logic).latestResultReturnsNullWhenNoBuckets— null/empty guard.Backend integration tests
KpiResourceITstill covers the endpoint).Manual testing performed
Reproduced via curl against a live instance:
latestKpiResultreturned0.0and equalledkpiResult(now-24h, now).results[0], whileresults[-1]held the correct0.68; polling confirmed it was structural, not a transient reindex.UI screen recording / screenshots:
Not applicable.
Checklist:
Greptile Summary
This PR changes KPI lookup to select the final result bucket. The main changes are:
Confidence Score: 4/5
The changed result-selection path can return an unrelated term bucket or an incomplete current-day bucket.
openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/KpiRepository.java
Important Files Changed
Reviews (1): Last reviewed commit: "fix(kpi): latestKpiResult returns most r..." | Re-trigger Greptile
Context used: