Skip to content

Study Landmarks package: comparison doc and profile command (#27)#101

Merged
lukstafi merged 1 commit intomainfrom
ludics/gh-ppx-minidebug-27-s2/root
Mar 25, 2026
Merged

Study Landmarks package: comparison doc and profile command (#27)#101
lukstafi merged 1 commit intomainfrom
ludics/gh-ppx-minidebug-27-s2/root

Conversation

@lukstafi
Copy link
Copy Markdown
Owner

Summary

  • Add LANDMARKS_COMPARISON.md documenting the overlap between ppx_minidebug and the Landmarks profiling library, including 6 concrete inspiration points and a recommendation against adding Landmarks as a dependency.
  • Add a profile command to minidebug_view that aggregates timing data by function name and location using SQL, inspired by Landmarks' aggregate_landmarks. Supports --format=json and --limit.
  • Add regression test for the profile command output.
  • Extend filter_test_output.ml to handle bare (non-angle-bracketed) time spans in test output.

Closes #27

Test plan

  • dune build compiles cleanly with no warnings
  • dune runtest passes all existing tests plus the new profile test
  • minidebug_view <db> profile produces correct aggregated output
  • minidebug_view <db> profile --format=json produces valid JSON
  • minidebug_view <db> profile --limit=1 limits output correctly

🤖 Generated with Claude Code

Study the Landmarks package (issue #27): add LANDMARKS_COMPARISON.md
documenting overlap, differences, and inspiration points. Implement
a `profile` command in minidebug_view that aggregates timing data
by function name using SQL, inspired by Landmarks' aggregate_landmarks.
Includes regression test and updated help text.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: a324ddb501

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

LEFT JOIN value_atoms va_loc ON e.location_value_id = va_loc.value_id
WHERE e.elapsed_end_ns IS NOT NULL
AND e.child_scope_id IS NOT NULL
AND e.seq_id = 0
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Drop seq_id=0 constraint in profile aggregation

In get_profiling_summary, filtering with e.seq_id = 0 makes the profile include only the first child entry of each parent scope, so sibling calls are silently excluded from call_count, total_ns, and ordering. This breaks accuracy whenever a function body performs multiple instrumented calls (or when root scope has multiple top-level calls), because only one of those calls is aggregated while the rest are ignored.

Useful? React with 👍 / 👎.

@lukstafi lukstafi merged commit c7e733d into main Mar 25, 2026
0 of 6 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.

Study the Landmarks package, describe overlap, find inspiration

1 participant