docs: Validate the pyroscope.* component topics - #7099
clayton-cornell wants to merge 18 commits into
Conversation
|
💻 Deploy preview available (docs: Validate the pyroscope.* component topics): |
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved documentation accuracy issues remain in the reviewed files.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
This documentation-only PR validates and standardizes the pyroscope.* component topics against current source behavior.
Changes:
- Adds review metadata and expands the namespace overview.
- Documents missing blocks, tracing, debug information, and metrics.
- Corrects formatting, examples, defaults, links, and behavior descriptions.
File summaries
| File | Summary |
|---|---|
docs/sources/reference/components/pyroscope/pyroscope.write.md |
Adds tracing and debug-upload documentation. |
docs/sources/reference/components/pyroscope/pyroscope.scrape.md |
Corrects scrape settings, examples, links, and metrics. |
docs/sources/reference/components/pyroscope/pyroscope.relabel.md |
Updates exported types and metric documentation. |
docs/sources/reference/components/pyroscope/pyroscope.receive_http.md |
Documents debug-information proxy behavior. |
docs/sources/reference/components/pyroscope/pyroscope.java.md |
Clarifies Java profiling and target behavior. |
docs/sources/reference/components/pyroscope/pyroscope.enrich.md |
Corrects argument types and configuration wording. |
docs/sources/reference/components/pyroscope/pyroscope.ebpf.md |
Documents debug information, metrics, and service behavior. |
docs/sources/reference/components/pyroscope/_index.md |
Adds metadata and namespace overview content. |
Review details
Suppressed comments (3)
docs/sources/reference/components/pyroscope/pyroscope.java.md:99
- This warning says the component “silently” skips the target, but the implementation logs
could not find PID labelbefore skipping it (internal/component/pyroscope/java/java.go:143-149). Removing “silently” avoids contradicting the behavior described in the preceding sentence.
It logs an error and silently skips profiling that target instead.
docs/sources/reference/components/pyroscope/pyroscope.receive_http.md:51
firstClient()iterates through allforward_toappendables and returns the first available debug-info client, so this is not necessarily the first receiver when an earlier receiver exposes no debug-info client. Please describe the selection as the first available downstream client inforward_toorder.
Debug information upload requests are only proxied to the first receiver in `forward_to`.
Unlike profiles, which are sent to every configured receiver, debug information isn't fanned out to the rest of the list.
docs/sources/reference/components/pyroscope/pyroscope.scrape.md:304
delta_profiling_durationoverrides the defaultscrape_interval - 1value for delta requests, as described in the common configuration section below. This unconditional wording is therefore inaccurate; document the override here as well.
The `seconds` used is equal to `scrape_interval - 1`.
- Files reviewed: 8/8 changed files
- Comments generated: 6
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🔵 Needs a closer look
One or more issues must be addressed before approval.
Review details
Suppressed comments (10)
Previously missed (2) — in code that hasn't changed since the last review.
docs/sources/reference/components/pyroscope/pyroscope.ebpf.md:55
- The compound modifier should be hyphenated here; “high-level languages” is the grammatical form used when modifying “languages.”
docs/sources/reference/components/pyroscope/pyroscope.ebpf.md:55 - The updated list uses
*markers, but the repository documentation style requires dashes for unordered lists (.docs/agent/style.md:127). Keep these markers as-so this cleanup doesn't introduce a formatting inconsistency.
This issue also appears on line 551 of the same file.
docs/sources/reference/components/pyroscope/pyroscope.ebpf.md:175
pyroscope.ebpfalso createspyroscope.NewFanout, so it exposespyroscope_forwarded_entries_totalalongside the metrics listed here. The new completeness-oriented table should include this counter; otherwise users cannot discover all metrics emitted by the component.
| `pyroscope_fanout_latency` | `histogram` | Write latency for sending to direct and indirect components. |
docs/sources/reference/components/pyroscope/pyroscope.ebpf.md:552
- This generated compatibility list is also being changed to
*markers, contrary to the repository documentation style requiring dashes for unordered lists (.docs/agent/style.md:127). Preserve the existing-markers here.
* Components that export [Targets](../../../compatibility/#targets-exporters)
* Components that export [Pyroscope `ProfilesReceiver`](../../../compatibility/#pyroscope-profilesreceiver-exporters)
docs/sources/reference/components/pyroscope/pyroscope.ebpf.md:284
- This now correctly documents the inferred name as
ebpf/<namespace>/<container_name>, but the Kubernetes example below still says the label is{namespace}/{container_name}even though its relabel rule emitsebpf/${1}/${2}. Please update that example text as well so the documented default and example agree.
* `__meta_kubernetes_namespace` and `__meta_kubernetes_pod_container_name`, combined into `ebpf/<namespace>/<container_name>`.
docs/sources/reference/components/pyroscope/pyroscope.java.md:192
- These subsections are now placed after the
### custom_argumentsheading but still use####, so the rendered hierarchy makeseventandper_threadchildren ofcustom_argumentseven though they are independentprofiling_configarguments listed in its table. Move them beforecustom_arguments(or otherwise make them sibling sections) so the headings and table have the same structure.
#### `event`
The `event` argument configures the profiling mode used by async-profiler.
async-profiler supports various profiling modes including CPU profiling, wall-clock profiling, and hardware performance monitoring events.
For a complete overview of all available profiling modes and their use cases, refer to [Profiling modes](https://github.com/async-profiler/async-profiler/blob/master/docs/ProfilingModes.md) in the async-profiler documentation.
#### `per_thread`
docs/sources/reference/components/pyroscope/pyroscope.receive_http.md:48
- This new paragraph introduces a debug-information proxy endpoint without naming its request path. The component mounts
POST /debuginfo.v1alpha1.DebuginfoService/Upload/{gnu_build_id}(internal/component/pyroscope/receive_http/receive_http.go:155), so include that path to make the new timeout behavior actionable.
`debug_info_upload_timeout` applies only to the debug information upload proxy endpoint `pyroscope.receive_http` exposes for downstream components.
It doesn't affect the profile-ingest endpoints described in [Usage](#usage).
docs/sources/reference/components/pyroscope/pyroscope.relabel.md:96
- The relabel component constructs the same
pyroscope.NewFanout, which exposes bothpyroscope_forwarded_entries_totalandpyroscope_fanout_latency. Since this section now claims to list the exposed Prometheus metrics, those shared fanout metrics should be included too.
| `pyroscope_relabel_profiles_written` | `counter` | Total number of profiles forwarded. |
docs/sources/reference/components/pyroscope/pyroscope.scrape.md:457
- This table is still incomplete:
pyroscope.scrapeconstructspyroscope.NewFanout, which registerspyroscope_forwarded_entries_totalin addition topyroscope_fanout_latency. Without documenting that counter, the new “following Prometheus metrics” section omits an exposed metric.
| `pyroscope_fanout_latency` | `histogram` | Write latency for sending to direct and indirect components. |
docs/sources/reference/components/pyroscope/pyroscope.write.md:172
- The latency metric's aggregate
push_totalandingest_totalseries useendpoint="-", not an endpoint URL (write.go:313andwrite.go:524). This sentence should call out that exception so users don't assume everyendpointlabel identifies a real destination.
All per-endpoint metrics include an `endpoint` label identifying the specific endpoint URL. The `pyroscope_ebpf_debug_info_upload_bytes_total` metric doesn't include any labels. The `pyroscope_write_latency` metric includes an additional `type` label with the following values:
- Files reviewed: 8/8 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved documentation corrections remain, including a missing metric label and behavior and formatting clarifications.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (5)
docs/sources/reference/components/pyroscope/pyroscope.ebpf.md:553
- This generated compatibility list is also changed from the repository's required dash style to asterisks. Please retain the dash markers here (and update the generator if this block is regenerated) rather than introducing a formatting regression.
* Components that export [Targets](../../../compatibility/#targets-exporters)
* Components that export [Pyroscope `ProfilesReceiver`](../../../compatibility/#pyroscope-profilesreceiver-exporters)
docs/sources/reference/components/pyroscope/pyroscope.java.md:99
- “Silently skips” contradicts the preceding statement that the component logs an error, and could lead operators to expect no diagnostic when a target lacks this label. Please say that it logs an error and skips profiling the target.
It logs an error and silently skips profiling that target instead.
docs/sources/reference/components/pyroscope/pyroscope.java.md:86
pyroscope.javais also built for Darwin (internal/component/pyroscope/java/java.go:1), where the implementation useslibasyncProfiler.dylibandCopyLibis a no-op (internal/component/pyroscope/java/asprof/asprof_darwin.go:14-23). This sentence still documents only the Linux.socopy behavior; please make the platform-specific behavior explicit so macOS users are not misled.
After process profiling startup, the component detects `libc` type and copies the matching `libasyncProfiler.so` into the target process's file system at the exact same path.
docs/sources/reference/components/pyroscope/pyroscope.scrape.md:171
- This changed unordered-list item still uses
*, while the repository style guide requires-(.docs/agent/style.md:127). Please use the dash marker here as well.
* Any scrape HTTP requests where the [delta argument][] is set to `true` fail, because they attempt to run for 59 seconds.
docs/sources/reference/components/pyroscope/pyroscope.scrape.md:304
- This is unconditional, but
targetsFromGroupusesdelta_profiling_durationinstead ofscrape_interval - 1when that argument is configured (internal/component/pyroscope/scrape/target.go:361-365). Please describescrape_interval - 1as the default and mention the override, otherwise the custom-profile documentation contradicts the component behavior and the common-configuration section below.
The `seconds` used is equal to `scrape_interval - 1`.
- Files reviewed: 8/8 changed files
- Comments generated: 3
- Review effort level: Lite
There was a problem hiding this comment.
🟡 Changes recommended
One or more issues must be addressed before approval.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (5)
Previously missed (2) — in code that hasn't changed since the last review.
docs/sources/reference/components/pyroscope/pyroscope.java.md:186
eventis an attribute of theprofiling_configblock (ProfilingConfig.Event), not a component-level section. This change promotes it from the original####heading to###, which makes the configuration hierarchy ambiguous and can suggest placingeventdirectly inpyroscope.java; keep it nested underprofiling_config.
This issue also appears on line 192 of the same file.
docs/sources/reference/components/pyroscope/pyroscope.receive_http.md:51
- The implementation flattens
DebugInfoClients()from allforward_toappendables and selects the first available client, so this is not necessarily the first configured receiver: an earlier receiver with no debug-info client causes a later receiver's endpoint to be selected. Please document the first available downstream endpoint rather than the first receiver.
docs/sources/reference/components/pyroscope/pyroscope.ebpf.md:55
- This changes the unordered-list marker from
-to*, but Alloy's documentation style guide requires dashes (.docs/agent/style.md:127). Please retain the existing dash markers here to avoid reintroducing a formatting violation.
* Native code (C/C++, Rust, Zig, Go, etc. without debug symbols on host)
* Broad set of high-level languages (HotSpot JVM, Python, Ruby, PHP, Node.js, V8, Perl).
docs/sources/reference/components/pyroscope/pyroscope.ebpf.md:553
- This changes the unordered-list marker from
-to*, but Alloy's documentation style guide requires dashes (.docs/agent/style.md:127). Please retain the existing dash markers in this generated compatibility list.
* Components that export [Targets](../../../compatibility/#targets-exporters)
* Components that export [Pyroscope `ProfilesReceiver`](../../../compatibility/#pyroscope-profilesreceiver-exporters)
docs/sources/reference/components/pyroscope/pyroscope.java.md:192
per_threadis also an attribute ofprofiling_config(ProfilingConfig.PerThread), but this new###heading places it at the block/component level instead of under that block. Restore the nested heading so users know where this argument is valid.
### `per_thread`
- Files reviewed: 8/8 changed files
- Comments generated: 1
- Review effort level: Lite
| | `proxy_url` | `string` | HTTP proxy to send requests through. | | no | | ||
| | `scheme` | `string` | The URL scheme with which to fetch profiles from targets. | `"http"` | no | | ||
| | `scrape_interval` | `duration` | How frequently to scrape the targets of this scrape configuration. | `"15s"` | no | | ||
| | `scrape_timeout` | `duration` | The timeout for scraping targets of this configuration. | `"10s"` | no | |
This PR is a general cleanup of the
pyroscope.*topics.This is preliminary work that needs to be done as part of an overall project to migrate Examples either to task topics or to scenarios. The foundation of the component doc needs to be in good shape before any other work is undertaken.
Validated against the component source code, looking at Blocks, Arguments, completeness, accuracy, etc. Most fixes are cosmetic (markdown formatting, removing extra spaces, aligning tables). Some fixes covered errors in the existing examples, errors in the debug section, and product name errors.
Added
review_datemetadata to all files that have been reviewed. This is something we should start using on all doc topics going forward once they are fully reviewed, and updated with subsequent reviews.Note for reviewers.
In the topic
pyroscope.ebpf, the bulk of the changes - especially tables - is just column alignment and alpha-sorting. I also added some Debug metrics that were missing (lines 167-169), and some missing Services (lines 268-272).The review exposed several undocumented pieces. Specifically:
I added a missing block to
debug_infoblock to thepyroscope.ebpfcomponent doc (lines 122-145)debug_infoblock to thepyroscope.ebpfcomponent, and it was added to the Alloy docs.I added a
tracingblock topyroscope.writecomponent doc (lines 131-140)pyroscope.write, but it wasn't documented.I added
pyroscope_ebpf_debug_info_upload_bytes_totalto thepyroscope.writecomponent.pyroscope_ebpf_debug_info_upload_bytes_totalto thepyroscope.writecomponent. I think it should be something more likepyroscope_write_debug_info_upload_bytes_total. It's documented with the ebpf naming since that's how it is implemented right now.I've contacted the dev who worked on these PRs for clarification - waiting on his verification (assuming i found the right person). For now it's documented as it exists in the source, but this may change depending on the answer.
Possible bug in source?
This is something Claude and CoPilot both found:
Bug:
pyroscope_ebpf_pprof_samples_totalcounts bytes, not samples.In
send.go, bothpprofSamplesTotalandpprofBytesTotalare incremented withlen(p.Raw), the same raw profile byte slice.pprof_samples_totalisn't tracking sample counts at all; it's a duplicate ofpprof_bytes_total. The metric's Help string inmetrics.gois also a stale copy-paste ("Total number of pprof profiles collected..."), unrelated to samples. This looks like a copy-paste artifact from when the metric was added rather than intentional behavior. Needs either real sample-count instrumentation or the metric renamed/removed to stop implying it counts samples. Docs updated to describe current (byte-counting) behavior in the meantime.