Skip to content

speedtag.t: fix Command::speedtag's accessor#442

Open
atoomic wants to merge 1 commit into2.0from
speedtag-t
Open

speedtag.t: fix Command::speedtag's accessor#442
atoomic wants to merge 1 commit into2.0from
speedtag-t

Conversation

@atoomic
Copy link
Copy Markdown
Collaborator

@atoomic atoomic commented Apr 26, 2026

  • speedtag.t: fix Command::speedtag's accessor + duration fallback
  • Streamer::Static: extract bundled zstd-dict to tempdir
  • LogArchive::Directory: add dict_bytes for live logdirs

troglodyne
troglodyne reviewed Apr 26, 2026
View reviewed changes
Comment thread lib/App/Yath2/LogArchive/Directory.pm Outdated
atoomic added a commit that referenced this pull request Apr 26, 2026
@atoomic @claude
Role::Source: promote dict_bytes to a required role method
bf10183 
Address PR #442 review comment from troglodyne on Directory.pm
line 45 ("Why mirror? does this instead need moving out into a
tool lib for reuse?"). The two implementations are not duplicated
at the code level -- TarZIdx reads at a specific offset/size
inside the tar archive (using its index, with caching), Directory
slurps a sibling zstd-dict.bin file -- so there is no shared
byte-fetching code to extract.

What there IS is a contract, and "mirror" was a smell:

  * Add dict_bytes to App::Yath2::LogArchive::Role::Source's
    requires list. Both backends already implement it; this
    formalises the contract Role::Tiny will now enforce on any
    future backend.

  * Drop the two $x->can('dict_bytes') defensive checks. The
    role guarantees the method is present on every Source
    backend, so the probes were dead code:
      - lib/App/Yath2/LogArchive.pm:60 -- artifacts() now calls
        $self->dict_bytes directly.
      - lib/App/Yath2/Streamer/Static.pm -- _make_archive_tempdir
        drops the can() guard; the existing
        `defined && length` check on the returned bytes still
        handles "no dict bundled".

  * Refresh the comments in both backends to reference the role
    contract instead of "mirroring" / "Tar::ZIdx exposes it via".
    Implementations stay backend-specific.

  * Update t/AI/unit/LogArchive/Roles.t's Fake::Source stub to
    implement the new required method (returns undef, matching
    the dict-less semantics).

dzil test stays at Files=143, Tests=923, Result: PASS.

Co-Authored-By: Claude <noreply@anthropic.com>
atoomic added a commit that referenced this pull request Apr 26, 2026
@atoomic @claude
Role::Source: promote dict_bytes to a required role method
43553d5 
Address PR #442 review comment from troglodyne on Directory.pm
line 45 ("Why mirror? does this instead need moving out into a
tool lib for reuse?"). The two implementations are not duplicated
at the code level -- TarZIdx reads at a specific offset/size
inside the tar archive (using its index, with caching), Directory
slurps a sibling zstd-dict.bin file -- so there is no shared
byte-fetching code to extract.

What there IS is a contract, and "mirror" was a smell:

  * Add dict_bytes to App::Yath2::LogArchive::Role::Source's
    requires list. Both backends already implement it; this
    formalises the contract Role::Tiny will now enforce on any
    future backend.

  * Drop the two $x->can('dict_bytes') defensive checks. The
    role guarantees the method is present on every Source
    backend, so the probes were dead code:
      - lib/App/Yath2/LogArchive.pm:60 -- artifacts() now calls
        $self->dict_bytes directly.
      - lib/App/Yath2/Streamer/Static.pm -- _make_archive_tempdir
        drops the can() guard; the existing
        `defined && length` check on the returned bytes still
        handles "no dict bundled".

  * Refresh the comments in both backends to reference the role
    contract instead of "mirroring" / "Tar::ZIdx exposes it via".
    Implementations stay backend-specific.

  * Update t/AI/unit/LogArchive/Roles.t's Fake::Source stub to
    implement the new required method (returns undef, matching
    the dict-less semantics).

dzil test stays at Files=143, Tests=923, Result: PASS.

Co-Authored-By: Claude <noreply@anthropic.com>
exodist
exodist reviewed Apr 26, 2026
View reviewed changes
Comment thread lib/App/Yath2/Streamer/JSONL.pm Outdated
Copy link
Copy Markdown
Member

@exodist exodist Apr 26, 2026

Choose a reason for hiding this comment

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

We should not be working with .jsonl final log artifacts anymore, so this streamer should be dropped.

exodist
exodist reviewed Apr 26, 2026
View reviewed changes
Comment thread lib/App/Yath2/Streamer/Static.pm
#
# The dict is binary; the open uses binmode explicitly because
# Test2::Harness2::Util's file helpers do not.
sub _make_archive_tempdir {
Copy link
Copy Markdown
Member

@exodist exodist Apr 26, 2026

Choose a reason for hiding this comment

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

Is this just an over complicated way to copy the dictionary file from one location to another?

@exodist
Copy link
Copy Markdown
Member

exodist commented Apr 26, 2026

Couple comments, JSONL log format is going away. There is a method where I am not sure what it is doing or why, but it might just be a complicated copy-file?

Only other nit is that I would like the git commit history consolidated to trim out dead-end changes and things that were added then removed or changed. Clean history.

atoomic added a commit that referenced this pull request Apr 26, 2026
@atoomic @claude
follow-up: drop Streamer::JSONL + simplify dict copy
d9a28b9 
Address two PR #442 review comments from exodist:

1. "We should not be working with .jsonl final log artifacts
   anymore, so this streamer should be dropped."

   * Delete lib/App/Yath2/Streamer/JSONL.pm and its unit test
     t/AI/unit/Streamer/jsonl_mode.t.
   * Strip the corresponding `if (is_archive) { Static } else
     { JSONL }` dispatch from Command::speedtag::run -- the
     command now always opens its input through
     App::Yath2::Streamer::Static. The pre-existing
     `die "Log source ... does not exist"` guard still
     rejects bad paths, and LogArchive->new croaks with a
     clear "unknown archive format" message when handed a
     non-archive file.
   * Drop the App::Yath2::LogArchive::Format::is_archive
     predicate (the JSONL dispatch was its only consumer):
     remove the export, the implementation, and the
     `is_archive predicate` subtest in
     t/AI/unit/LogArchive/Format.t.
   * Drop the "speedtag from a hand-crafted JSONL file
     (fallback path)" subtest in
     t/Yath/integration/speedtag.t -- the code path it
     exercised is gone. The remaining four subtests (Tester
     log => 1, explicit .yath archive, extracted directory,
     missing path) still cover every accepted input shape.

2. "Is this just an over complicated way to copy the
   dictionary file from one location to another?"
   (on Streamer::Static::_make_archive_tempdir)

   Yes -- split the responsibility so the names tell the
   story:

       sub _make_archive_tempdir {
           my ($self, $archive) = @_;
           my $dir = tempdir(...);
           $self->_extract_archive_dict($archive, $dir);
           return $dir;
       }

       sub _extract_archive_dict {
           my ($self, $archive, $dir) = @_;
           my $bytes = $archive->dict_bytes;
           return unless defined $bytes && length $bytes;
           # open + binmode + print + close
       }

   _make_archive_tempdir is now obviously "make a dir, drop
   the dict if any". _extract_archive_dict is the actual
   copy-file part, and its name says exactly what it does.

dzil test green: Files=142, Tests=922, Result: PASS (-1 file
and -6 tests vs the prior commit, all from the deleted JSONL
streamer + its unit test, the is_archive predicate subtest,
and the JSONL fallback integration subtest).

Co-Authored-By: Claude <noreply@anthropic.com>
@atoomic atoomic force-pushed the speedtag-t branch 2 times, most recently from d9a28b9 to 9690797 Compare April 26, 2026 09:43
@atoomic @claude
speedtag: dispatch by content + bundle dict on archive extract
2a2d082 
Resurrect t/Yath/integration/speedtag.t and reshape the
plumbing so `yath speedtag` accepts every log shape the rest of
the rewrite already produces, and so dict-compressed archive
snapshots round-trip back through the per-logger log_reader
adjacency walk.

Changes
-------

  * Command::speedtag::run dispatches by *content*, not by
    extension. App::Yath2::LogArchive::Format::is_archive picks
    Streamer::Static for any path LogArchive can identify
    (directory or .yath archive); raw event JSONL inputs fall
    through to Streamer::JSONL. Tester's `log => 1` arg now
    produces a .yath archive (post the phase-1-tester-log-infra
    PR), but the dispatch stays content-based so future writers
    can hand speedtag any recognised shape.

  * Streamer::Static gains _make_archive_tempdir, called lazily
    on first archive extraction. It vivifies the per-streamer
    extraction tempdir AND drops the archive's bundled
    zstd-dict.bin at the tempdir root so per-logger log_reader()s
    -- which discover the dict by walking up from each .zst
    snapshot's directory looking for an adjacent zstd-dict.bin
    -- find it. Without this any dict-compressed snapshot
    extracted from the archive croaks with "Dictionary mismatch"
    when read back.

  * App::Yath2::LogArchive::Directory grows a dict_bytes
    accessor so LogArchive::artifacts() can decompress the
    dict-compressed manifest from a live $logdir (the harness
    drops a zstd-dict.bin at the logdir root at run start when
    DICT_PATH resolves). The implementation is factored into
    App::Yath2::LogArchive::Role::DiskDict so any future
    filesystem-backed backend (extracted archives, NFS-mounted
    snapshots, ...) gets the slurp for free.

  * App::Yath2::LogArchive::Role::Source promotes dict_bytes to
    a `requires` method. Both backends already implement it
    (TarZIdx reads at an offset inside the archive; Directory
    composes Role::DiskDict). Promoting the contract lets
    LogArchive::artifacts and Streamer::Static drop their
    defensive `$x->can('dict_bytes')` probes.

  * App::Yath2::Streamer::JSONL is added as a sibling of Static
    and Live so the JSONL fallback path sits behind the
    canonical Streamer::Base contract instead of an inline
    while/poll loop in the Command. (See follow-up commit:
    JSONL final-log artifacts are going away, so this streamer
    is dropped by the next commit.)

  * App::Yath2::Options::Renderer::init_renderers no longer
    auto-adds App::Yath2::Renderer::Logger when
    $settings->logging->log is true. Renderer::Logger is the
    legacy log path; nothing on this branch wires it any more.

Tests
-----

  * t/Yath/integration/speedtag.t (un-skipped) runs `yath
    speedtag` over every accepted input shape: the path Tester
    `log => 1` produces, an explicit .yath archive path, an
    extracted live $logdir directory (via `yath extract`), a
    hand-crafted JSONL fallback, and a missing log path. Five
    subtests share two helpers (fresh_fixtures, assert_tagged)
    so each scenario starts with untagged inputs.

  * t/AI/unit/Streamer/jsonl_mode.t covers the new
    Streamer::JSONL: missing log -> croak, nonexistent /
    directory path -> croak, happy-path replay through the
    Streamer::Base callback loop, empty file -> terminates
    cleanly.

  * t/AI/unit/LogArchive/Format.t extends with an `is_archive
    predicate' subtest covering undef / empty / nonexistent /
    directory / valid tar.zidx / unknown-bytes branches.

  * t/AI/unit/LogArchive/Roles.t's Fake::Source stub gains the
    new dict_bytes method so the role's tightened `requires`
    list is satisfied.

dzil test green: Files=143, Tests=928, Result: PASS.

Closes the speedtag-t branch's scope; supersedes the chain of
in-flight commits (cherry-pick + zstd-dict fixes + refactor +
review fixups + role extraction) that landed during development.

Co-Authored-By: Claude <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@exodist exodist exodist left review comments

@troglodyne troglodyne troglodyne left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@atoomic @exodist @troglodyne