docs: README#1
Open
necco-c wants to merge 1 commit into
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Reviewer notes for grpcsnoop
✅ Reviewer action items
discord.gg/bBRF9d2Gw2, which is different from the canonical yeet invite (discord.gg/dYZu9PjKB) used in the new footer. If the badge is intentionally a separate channel (grpcsnoop-specific?), fine; otherwise unify it with the footer link.(extrapolated, grounding N — review)tags from the README in the same commit.tcx(the existing README also stated 6.6; consistent)LICENSEfile —grpcsnoop.bpf.cdeclaresSPDX-License-Identifier: GPL-2.0andSEC("license") = "GPL", but the repo has no top-levelLICENSEfile and the JS modules have no license headers. Pick a license and add the file.yeet run github:yeet-src/grpcsnoop -- --port 50051first, then offersgit clone + bash demo/up.shas the second path for trying the bundled demo. The old README led withmakeand ranyeet run . --port 50051against the demo. Confirm the new ordering (shorthand first, demo second) is right.About description
(101 chars)
Topic tags
The obvious-search cluster is
ebpf,bpf,linux,grpc,protobuf,http2,observability.tcxandtccarry the specific kernel-attach-point language (and match what upstreamtop ships, for consistency).containersis genuinely load-bearing here because the tool's whole premise is east-west container traffic.yeetandkernelcarry the corpus-wide language.Flagged for review (grounding ≥ 2)
tcxattach viaBPF_LINK_TYPE_TCXmerged in 6.6, so the floor is honest. Source code itself doesn't gate on a version. Safe to ship.grpcsnoop.bpf.cdeclaresSPDX-License-Identifier: GPL-2.0andSEC("license") = "GPL". The repo has no top-levelLICENSEfile and the JS modules have no license headers. Stating that "the JS side has no separate license declaration in the source" is consistent with the code state; cleanest resolution is to add aLICENSEfile (see action item above).Source-level cleanup worth resolving in the repo (not in the README)
LICENSEfile. BPF is GPL by source declaration; JS side is unstated. Same pattern as ciprof, upstreamtop, poolnarc — worth a corpus-wide pass at some point.Makefiledefaults tosudo bpftoolfor thevmlinux.hgeneration (line 4). That's the build-time bpftool, separate from theyeet runflow, but worth flagging because the template's "nosudo yeetexamples" rule generalizes loosely to "the README shouldn't show sudo unless genuinely required" — and the Quick start now omits themakestep entirely (usesgithub:shorthand), so the sudo is hidden from the happy path. Good. Themakepath inside the "Building from source" section keeps it.grpcsnoop.bpf.cisSPDX-License-Identifier: GPL-2.0(header), whileSEC("license") = "GPL"is the runtime declaration. Both are present and consistent.Template-level observations
Possible new mode worth promoting: "medium-touch." This run sat between the default full-rewrite mode (used for ciprof, upstreamtop, the first upstreamtop pass) and the light-touch mode (used for poolnarc when explicitly requested). The existing grpcsnoop README was strong enough that a full rewrite would have destroyed value, but it wasn't explicitly flagged as "preserve content."
The pattern that emerged: when the existing README already passes template guards (one-sentence definition ≤ 25 words, has a 60-second primer, has honest caveats, has community FAQ, has a tagline), default to medium-touch: preserve substance, normalize structure to template order, swap installation/footer to the new conventions, tighten in-sentence em-dashes.
Worth a template bump bullet under "Preferences captured from prior reviews": something like "When the existing README already covers the template's mandatory sections with substantive content, default to medium-touch — preserve the prose inside each section, reorder to template order, swap installation/footer to current conventions, tighten in-sentence em-dashes. Don't full-rewrite a strong README just because the default mode is full-rewrite."