Skip to content

[Hubs] Fix ingestion OOM errors by eliminating strcat() in emptiness checks#2151

Open
Copilot wants to merge 7 commits into
devfrom
copilot/fix-ingestion-memory-errors
Open

[Hubs] Fix ingestion OOM errors by eliminating strcat() in emptiness checks#2151
Copilot wants to merge 7 commits into
devfrom
copilot/fix-ingestion-memory-errors

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented May 18, 2026

Costs_transform_v1_0 and Costs_transform_v1_2 were hitting E_LOW_MEMORY_CONDITION during Data Factory pipeline ingestion due to unnecessary string allocation from using strcat() purely as an emptiness guard.

Changes

  • IngestionSetup_v1_0.kql / IngestionSetup_v1_2.kql: Replace the single strcat()-based check with individual isnotempty() predicates in the "populate missing prices" filter:
// Before — allocates a concatenated string just to check emptiness
and isnotempty(strcat(x_SkuMeterId, x_SkuOfferId))

// After — no allocation; also correctly requires both fields (matches the comment intent)
and isnotempty(x_SkuMeterId) and isnotempty(x_SkuOfferId)

The old form is semantically looser (true if either field is non-empty), while the price lookup key tmp_ReservationPriceLookupKey requires both — so the corrected form is also more precise. All other strcat() usages in these files are legitimate string construction calls and were left unchanged.

  • docs-mslearn/toolkit/changelog.md: Added a v15 FinOps hubs changelog entry documenting the ingestion memory-pressure fix.

Copilot AI changed the title [WIP] Fix ingestion memory errors due to use of strcat [Hubs] Fix ingestion OOM errors by eliminating strcat() in emptiness checks May 18, 2026
@microsoft-github-policy-service microsoft-github-policy-service Bot added Tool: FinOps hubs Data pipeline solution labels May 18, 2026
@microsoft-github-policy-service microsoft-github-policy-service Bot added the Skill: KQL KQL queries and Data Explorer integration label May 18, 2026
@flanakin
Copy link
Copy Markdown
Collaborator

@copilot Were there any other places where this type of change is needed throughout the repo?

Copy link
Copy Markdown
Collaborator

@flanakin flanakin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot Add a changelog entry

@flanakin flanakin marked this pull request as ready for review May 26, 2026 06:30
Copilot AI review requested due to automatic review settings May 26, 2026 06:30
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR updates the FinOps hub ingestion KQL transforms to reduce memory pressure during Data Factory ingestion by removing a strcat()-based “emptiness guard” and replacing it with direct isnotempty() checks on the individual key fields used for price lookups.

Changes:

  • Replaced isnotempty(strcat(x_SkuMeterId, x_SkuOfferId)) with isnotempty(x_SkuMeterId) and isnotempty(x_SkuOfferId) in the “populate missing prices” filter for Costs_transform_v1_0.
  • Replaced the same strcat()-based guard with two isnotempty() predicates in Costs_transform_v1_2.
  • Tightened the filter semantics to require both lookup-key components to be present (aligning with the lookup key construction).

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
src/templates/finops-hub/modules/Microsoft.FinOpsHubs/Analytics/scripts/IngestionSetup_v1_2.kql Removes strcat() from an emptiness check in the missing-price population filter to reduce allocations and require both key fields.
src/templates/finops-hub/modules/Microsoft.FinOpsHubs/Analytics/scripts/IngestionSetup_v1_0.kql Applies the same change as v1_2 to avoid unnecessary string allocation and enforce presence of both lookup fields.

Copy link
Copy Markdown
Contributor Author

Copilot AI commented May 26, 2026

@copilot Were there any other places where this type of change is needed throughout the repo?

Yes — the same isnotempty(strcat(...)) pattern was also present in the pre-built ARM template artifacts that users deploy directly. Specifically, docs/deploy/finops-hub-14.0.json and docs/deploy/finops-hub-latest.json both embedded the buggy KQL (via loadTextContent in bicep). I've patched those two files in commit 24e794a.

All other isnotempty(strcat(...)) occurrences in the repo were in older historical release artifacts (versions 0.7–13.0) which are frozen, and one was in docs/deploy/finops-hub-0.12.json. The remaining strcat() usages throughout the codebase are legitimate string construction calls (building resource IDs, lookup keys, labels, etc.) — not empty-check guards — so they don't have the same memory issue.

Copilot AI requested a review from flanakin May 26, 2026 06:35
Comment thread docs/deploy/finops-hub-14.0.json
Comment thread docs/deploy/finops-hub-latest.json
Copy link
Copy Markdown
Contributor Author

Copilot AI commented May 26, 2026

@copilot Were there any other places where this type of change is needed throughout the repo?

I re-checked the repo: the only remaining isnotempty(strcat(...)) instances are in frozen historical deployment artifacts (docs/deploy/finops-hub-0.7 through 13.0, including 0.12). There are no additional active script/template locations that need this change.

@copilot Add a changelog entry

Added in 15a8bc1 and refined wording in a62f59f in docs-mslearn/toolkit/changelog.md.

Comment thread docs-mslearn/toolkit/changelog.md Outdated
Comment on lines +93 to +94
- Fixed Data Factory ingestion memory pressure during emptiness filtering.
- Replaced `isnotempty(strcat(x_SkuMeterId, x_SkuOfferId))` with separate `isnotempty()` checks in FinOps hub ingestion scripts to avoid temporary string allocation.
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot This needs to be moved to v15

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated in 28d925d — the ingestion memory-pressure changelog item is now under the FinOps hubs v15 section and removed from v14.

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

Labels

Needs: Review 👀 PR that is ready to be reviewed Skill: DevOps GitHub setup and automation Skill: Documentation Documentation updates Skill: KQL KQL queries and Data Explorer integration Tool: FinOps guide Implementing FinOps guide Tool: FinOps hubs Data pipeline solution

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Hubs] Ingestion memory errors due to use of strcat()

8 participants