Skip to content

GH-140: Remove cloud account id#141

Merged
s-prosvirnin merged 2 commits into
mainfrom
GH-140/remove-cloud-account-id
May 17, 2026
Merged

GH-140: Remove cloud account id#141
s-prosvirnin merged 2 commits into
mainfrom
GH-140/remove-cloud-account-id

Conversation

@s-prosvirnin
Copy link
Copy Markdown
Member

@s-prosvirnin s-prosvirnin commented May 16, 2026

Summary by CodeRabbit

  • New Features

    • Added a Tempo datasource to Grafana for tracing queries.
  • Refactor

    • Simplified multi-tenancy to use tenant_id only; removed cloud_account_id as a dedicated top-level column across tables.
    • Updated sorting/partitioning and query label handling to match the new schema.
  • Documentation

    • Revised schema docs and guidance to reflect v1.3 changes and new sort/key recommendations.

Review Change Stack

@s-prosvirnin s-prosvirnin requested review from a team and frisbeeman May 16, 2026 23:27
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 16, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 91d589e0-7247-4295-a83e-bcf2e26c8cc7

📥 Commits

Reviewing files that changed from the base of the PR and between 8fd2866 and 212fe5e.

📒 Files selected for processing (1)
  • crates/icegate-query/benches/common/harness.rs
💤 Files with no reviewable changes (1)
  • crates/icegate-query/benches/common/harness.rs

Walkthrough

Remove cloud_account_id from schemas and transforms; move cloud.account.id into attributes MAP. Update sort orders to service_name-first, adjust WAL boundary/sorter/shift logic, remove cloud_account_id from query pushdown/indexed-label mappings, update tests, and add a Grafana Tempo datasource entry.

Changes

Schema redefinition and OTLP→Arrow transform

Layer / File(s) Summary
Schema definitions and exported constants
crates/icegate-common/src/SCHEMA.md, crates/icegate-common/src/parquet_encoding.rs, crates/icegate-common/src/schema.rs
Remove cloud_account_id from logs/spans/events/metrics schemas, renumber field IDs, and change sort orders to service_name-first. Remove COL_CLOUD_ACCOUNT_ID and update indexed/label constants and schema docs (v1.3).
OTLP→Arrow transform logic
crates/icegate-ingest/src/transform.rs
Stop promoting cloud.account.id as a top-level column for logs and spans; keep service.name promoted. cloud.account.id is now written into the attributes MAP; unit tests updated accordingly.

WAL sorting, boundary keys, and shift partitioning

Layer / File(s) Summary
Row-group boundary key and range schema
crates/icegate-ingest/src/shift/plan_runner.rs, crates/icegate-ingest/src/shift/planner_partitioning.rs, crates/icegate-ingest/src/wal/boundary.rs
Boundary helpers and serialized names now use service_name+timestamp (drop account), updated test fixtures and JSON round-trip fixtures.
WAL sorter test fixtures and sorting logic
crates/icegate-ingest/src/wal/sorter.rs
Logs batch schema in tests removes cloud_account_id; boundary metadata and pre-sort assertions updated to validate service_name+timestamp ordering and metadata shapes.
Shift runner and row-group merger
crates/icegate-ingest/src/shift/shift_runner.rs, crates/icegate-ingest/src/shift/row_groups_merger.rs
Batch builders/signatures change from 4-tuple (account,service,timestamp,value) to 3-tuple (service,timestamp,value); column indices, row extraction, create_e2e_logs_table in tests, and related assertions adjusted.

Query optimization and label/tag discovery

Layer / File(s) Summary
Loki log formatting and label extraction
crates/icegate-query/src/loki/formatters.rs, crates/icegate-query/src/loki/predicate.rs, crates/icegate-query/src/engine/metadata_scan/values.rs
BatchColumns drops cloud_account_id; extract_labels no longer inserts it; test configs and metadata-scan indexed_columns updated to exclude cloud_account_id.
Tempo span metadata and tag discovery
crates/icegate-query/src/tempo/metadata.rs, crates/icegate-query/src/tempo/models.rs
Resource-scope top-level tag mapping limited to service.name; map_attribute_to_column removed special-case for cloud.account.id so it falls back to MAP lookup; docs/comments updated.
TraceQL predicate translation
crates/icegate-query/src/traceql/datafusion/selectors.rs, crates/icegate-query/src/traceql/iceberg_predicate.rs
Remove cloud.account.id -> top-level column mapping; predicates referencing it are dropped to AlwaysTrue for pushdown rules; tests adjusted to expect drop.

Query test fixtures and integration tests

Layer / File(s) Summary
Loki test batches and harness
crates/icegate-query/tests/loki/*
Remove cloud_account_id from test RecordBatch builders; adjust attributes MAP index and queries (use severity_text/node instead of cloud_account_id); label discovery asserts cloud_account_id absent.
Tempo test batches and span-scope assertions
crates/icegate-query/tests/tempo/*
Span batch builders remove cloud_account_id; span-scope tag assertions updated accordingly.
Queue integration tests
crates/icegate-queue/tests/*
logs_batch signature updated to 3-tuple; boundary metadata builders and field extractor tests switch from account to service; WAL footer roundtrip tests updated.
Benchmarks and harnesses
crates/icegate-query/benches/*
Synthetic benchmark batch builders no longer generate account_id column or arrays.

Grafana provisioning

Layer / File(s) Summary
Grafana Tempo datasource
config/docker/grafana/provisioning/datasources/datasources.yaml
Added a non-default, proxy-access, non-editable Tempo datasource pointing to http://query:3200.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related issues

Possibly related PRs

Suggested reviewers

  • frisbeeman
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title directly and clearly describes the main change: removal of cloud account id across the codebase, which is the primary objective of this PR.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch GH-140/remove-cloud-account-id

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@crates/icegate-common/src/SCHEMA.md`:
- Around line 478-485: Update the v1.3 release note about where cloud.account.id
is stored: change the sentence that currently states "cloud.account.id resource
attribute now lives in the generic attributes MAP only" to clarify that spans
now split attributes into resource_attributes and span_attributes, and that
cloud.account.id has been moved into resource_attributes (and/or the generic
attributes MAP when not associated with a span). Edit the SCHEMA.md v1.3 section
(the "Notable Changes in v1.3" block mentioning cloud.account.id) to reference
the new fields resource_attributes and span_attributes and explicitly state the
storage location for cloud.account.id across logs, spans, and metrics.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: baae94ba-d1b3-405e-a78b-44b2339cbc20

📥 Commits

Reviewing files that changed from the base of the PR and between 98e67a3 and 8fd2866.

📒 Files selected for processing (27)
  • config/docker/grafana/provisioning/datasources/datasources.yaml
  • crates/icegate-common/src/SCHEMA.md
  • crates/icegate-common/src/parquet_encoding.rs
  • crates/icegate-common/src/schema.rs
  • crates/icegate-ingest/src/shift/plan_runner.rs
  • crates/icegate-ingest/src/shift/planner_partitioning.rs
  • crates/icegate-ingest/src/shift/row_groups_merger.rs
  • crates/icegate-ingest/src/shift/shift_runner.rs
  • crates/icegate-ingest/src/transform.rs
  • crates/icegate-ingest/src/wal/boundary.rs
  • crates/icegate-ingest/src/wal/sorter.rs
  • crates/icegate-query/src/engine/metadata_scan/values.rs
  • crates/icegate-query/src/loki/formatters.rs
  • crates/icegate-query/src/loki/predicate.rs
  • crates/icegate-query/src/tempo/metadata.rs
  • crates/icegate-query/src/tempo/models.rs
  • crates/icegate-query/src/traceql/datafusion/selectors.rs
  • crates/icegate-query/src/traceql/iceberg_predicate.rs
  • crates/icegate-query/tests/loki/grouping.rs
  • crates/icegate-query/tests/loki/harness.rs
  • crates/icegate-query/tests/loki/labels.rs
  • crates/icegate-query/tests/loki/unwrap.rs
  • crates/icegate-query/tests/tempo/harness.rs
  • crates/icegate-query/tests/tempo/tags.rs
  • crates/icegate-queue/tests/common/mod.rs
  • crates/icegate-queue/tests/reader_integration.rs
  • crates/icegate-queue/tests/writer_integration.rs
💤 Files with no reviewable changes (1)
  • crates/icegate-query/tests/tempo/harness.rs

Comment thread crates/icegate-common/src/SCHEMA.md
@s-prosvirnin s-prosvirnin merged commit 474ca52 into main May 17, 2026
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants