Skip to content

refactor: clean up code structure and improve comments across files #2523

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
tolgaozen merged 1 commit into from
Oct 7, 2025
Merged

refactor: clean up code structure and improve comments across files #2523

tolgaozen merged 1 commit into from
Oct 7, 2025
+951 −1,033

Conversation

@tolgaozen
Copy link
Member

@tolgaozen tolgaozen commented Oct 7, 2025 •
edited by coderabbitai bot
Loading

Summary by CodeRabbit

  • Bug Fixes

    • More reliable schema loading from URL, file, or inline sources with clearer error reporting.
    • Postgres storage: added per-write size validation and improved retry/backoff to reduce transient write failures.
    • Telemetry: unsupported meter exporter now returns a clear configuration error.
    • Clearer error messages in attribute parsing and GC logs.

  • Documentation

    • Updated SDK README (Go/Python sections) for improved clarity.

  • Tests

    • Reorganized memory storage and DSL parser tests for readability; no behavior changes.

  • Refactor/Style

    • Broad non-functional logging and comment improvements across authentication, storage, and telemetry.

@coderabbitai
Copy link

coderabbitai bot commented Oct 7, 2025 •
edited
Loading

Caution

Review failed

The pull request is closed.

Walkthrough

This PR is predominantly comment and logging refinements across the codebase. Notable functional modifications include: added validations and restructured batch helpers in Postgres data writer, enhanced logging paths, and introducing/clarifying an unsupported-default path in meter exporter factory. The rest are non-functional updates in tests, docs, telemetry logs, and minor import annotations.

Changes

Cohort / File(s) Summary
CI Workflow
.github/workflows/scorecard.yml		 Added a comment block labeling the permissions section; no behavioral change.
AuthN OpenID
internal/authn/openid/authn.go		 Comment and log message wording updates; no control-flow or API changes.
Engines
internal/engines/bulk.go		 Added inline comments to function signature and a call site; logic unchanged.
DB Factory & Migration
internal/factories/database.go, internal/storage/migration.go		 Comment clarifications around Postgres version checks; behavior unchanged.
Storage Context
internal/storage/context/attributes.go, internal/storage/context/tuples.go, internal/storage/context/tuples_test.go		 Added inline comments (e.g., on slices import); no logic changes.
Storage Memory (readers/writers/tests/constants/migrations/utils)
internal/storage/memory/*, internal/storage/memory/constants/constants.go, internal/storage/memory/migrations/schema.go, internal/storage/memory/utils/filter.go		 Comment-only refactors and test scaffolding improvements (Ginkgo/Gomega), annotations across reader/writer/txn paths; no functional changes.
Storage Postgres (data path)
internal/storage/postgres/data_reader.go, internal/storage/postgres/data_writer.go		 Reader: added structured logs; no logic changes. Writer: added data size validation, retry/backoff annotations, transaction ID logging, and refactored batch operations into helpers; control flow expanded but public API unchanged.
Storage Postgres (GC/Schema/Tenant/Watch)
internal/storage/postgres/gc/gc.go, internal/storage/postgres/schema_reader.go, internal/storage/postgres/schema_writer.go, internal/storage/postgres/tenant_reader.go, internal/storage/postgres/tenant_writer.go, internal/storage/postgres/watch.go		 Logging/message phrasing changes and inline comments; no behavior changes.
Attributes & Tuples
pkg/attribute/attribute.go, pkg/tuple/tuple.go		 Error message text refinements (attribute parsing); added import comments; no API changes.
CLI
pkg/cmd/flags/coverage.go, pkg/cmd/version.go		 Added comments around flags and version command; no functional changes.
Database Postgres Client
pkg/database/postgres/postgres.go		 Comment on slog import; no changes to behavior.
DSL (AST/Lexer/Parser tests)
pkg/dsl/ast/references.go, pkg/dsl/lexer/lexer.go, pkg/dsl/parser/parser_test.go		 Added clarifying comments; parser tests reformatted; no behavior changes.
Schema Loader
pkg/schema/loader.go, pkg/schema/loader_test.go		 Loader code annotated and organized; URL/file/inline helpers clarified; signatures unchanged; likely same behavior with clearer flow. Tests reformatted with comments.
Telemetry (Meter/Tracer/Exporters)
pkg/telemetry/meter.go, pkg/telemetry/tracer.go, pkg/telemetry/logexporters/factory.go, pkg/telemetry/meterexporters/factory.go, pkg/telemetry/tracerexporters/factory.go		 Comment and log text updates. Meter exporters factory documents and adds/clarifies default unsupported-error path; no change to supported cases.
SDK Docs
sdk/README.md		 Formatting and comments for SDK sections; content intact.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  actor Client
  participant Writer as PostgresDataWriter
  participant DB as Postgres
  participant Helpers as BatchHelpers

  rect rgb(238, 245, 255)
    note right of Writer: New/changed flow
    Client->>Writer: Write/Delete/RunBundle(request)
    Writer->>Writer: Validate sizes (tuples/attributes)
    Writer->>DB: BEGIN (tx)
    Writer->>DB: SELECT txid_current() // get transaction ID
    alt Write flow
      Writer->>Helpers: build batches (rels/attrs)
      Helpers-->>Writer: batched ops
      loop per batch
        Writer->>DB: EXEC batch
        DB-->>Writer: result or serialization error
        opt on serialization error
          Writer->>Writer: backoff wait and retry
        end
      end
    else Delete flow
      Writer->>Helpers: build delete clauses
      Writer->>DB: EXEC deletes
    end
    Writer->>DB: COMMIT
    Writer-->>Client: Encoded snapshot token
  end
Loading 
sequenceDiagram
  autonumber
  actor App
  participant Factory as MeterExporterFactory

  App->>Factory: ExporterFactory(kind, cfg)
  alt Supported kind
    Factory-->>App: exporter instance
  else Unsupported kind
    note right of Factory: Default case
    Factory-->>App: error (unsupported exporter)
  end
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related PRs

Suggested reviewers

  • tolgaOzen

Poem

I thump my foot—logs aligned just right,
Batches hop in tidy rows, day to night.
Tokens twinkle, snapshots glow,
Factories choose where metrics go.
With whiskers twitching, I approve the write—
Commit, and bound into the light! 🐇✨

✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch refactor-storage-memory-postgres
📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between d04dc6e and 84bf803.

📒 Files selected for processing (46)
  • .github/workflows/scorecard.yml (1 hunks)
  • internal/authn/openid/authn.go (9 hunks)
  • internal/engines/bulk.go (1 hunks)
  • internal/factories/database.go (2 hunks)
  • internal/storage/context/attributes.go (1 hunks)
  • internal/storage/context/tuples.go (1 hunks)
  • internal/storage/context/tuples_test.go (1 hunks)
  • internal/storage/memory/bundle_reader.go (2 hunks)
  • internal/storage/memory/bundle_reader_test.go (1 hunks)
  • internal/storage/memory/bundle_writer.go (2 hunks)
  • internal/storage/memory/bundle_writer_test.go (1 hunks)
  • internal/storage/memory/constants/constants.go (1 hunks)
  • internal/storage/memory/data_reader.go (4 hunks)
  • internal/storage/memory/data_writer.go (2 hunks)
  • internal/storage/memory/memory_test.go (1 hunks)
  • internal/storage/memory/migrations/schema.go (5 hunks)
  • internal/storage/memory/schema_reader.go (4 hunks)
  • internal/storage/memory/schema_writer.go (2 hunks)
  • internal/storage/memory/tenant_reader.go (2 hunks)
  • internal/storage/memory/tenant_writer.go (2 hunks)
  • internal/storage/memory/utils/filter.go (1 hunks)
  • internal/storage/migration.go (2 hunks)
  • internal/storage/postgres/data_reader.go (18 hunks)
  • internal/storage/postgres/data_writer.go (17 hunks)
  • internal/storage/postgres/gc/gc.go (2 hunks)
  • internal/storage/postgres/schema_reader.go (4 hunks)
  • internal/storage/postgres/schema_writer.go (3 hunks)
  • internal/storage/postgres/tenant_reader.go (4 hunks)
  • internal/storage/postgres/tenant_writer.go (3 hunks)
  • internal/storage/postgres/watch.go (9 hunks)
  • pkg/attribute/attribute.go (4 hunks)
  • pkg/cmd/flags/coverage.go (1 hunks)
  • pkg/cmd/version.go (1 hunks)
  • pkg/database/postgres/postgres.go (1 hunks)
  • pkg/dsl/ast/references.go (5 hunks)
  • pkg/dsl/lexer/lexer.go (1 hunks)
  • pkg/dsl/parser/parser_test.go (36 hunks)
  • pkg/schema/loader.go (2 hunks)
  • pkg/schema/loader_test.go (4 hunks)
  • pkg/telemetry/logexporters/factory.go (1 hunks)
  • pkg/telemetry/meter.go (2 hunks)
  • pkg/telemetry/meterexporters/factory.go (1 hunks)
  • pkg/telemetry/tracer.go (2 hunks)
  • pkg/telemetry/tracerexporters/factory.go (1 hunks)
  • pkg/tuple/tuple.go (1 hunks)
  • sdk/README.md (1 hunks)

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.

@tolgaozen tolgaozen merged commit 3ad63e4 into master Oct 7, 2025
8 of 10 checks passed
@tolgaozen tolgaozen deleted the refactor-storage-memory-postgres branch October 7, 2025 22:08
@coderabbitai coderabbitai bot mentioned this pull request Oct 10, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@tolgaozen