Skip to content

fix(metrics): address PR #38 review findings and refresh the dashboard - #39

Merged
MaurUppi merged 6 commits into
mainfrom
fix/metrics-review-findings
Oct 3, 2026
Merged

MaurUppi merged 6 commits into
mainfrom
fix/metrics-review-findings

Conversation

@MaurUppi

@MaurUppi MaurUppi commented Oct 3, 2026

Copy link
Copy Markdown
Owner

Background

This PR fixes the P0–P2 findings from a review of the metrics code merged in #38. The same metrics code is open upstream as daeuniverse#1015, and the 40 metrics files are byte-identical between the two. The same fix commit has already been pushed to the daeuniverse#1015 head branch feat/metrics-endpoint-clean (5c9df67).

The review report, with evidence and rationale for each finding, is in .plan/metrics/pr38-upstream-1015-review.md.

Checklist

Full Changelogs

  • 3c02657 fix(metrics): address review findings
    • P0 Remove dae_node_latency_seconds and dae_node_alive. Their link label exported the node share link, which embeds the Trojan password, the SS cipher:password, or the VLESS/VMess ID.
    • P0 Same-named nodes in one group made promhttp fail the whole scrape with HTTP 500. Dialer labels now get a #N suffix, and the handler uses ContinueOnError.
    • P1 Export each health collection once, via dialer.StandardHealthKeys(). tcp4(DNS)/tcp6(DNS) alias tcp4/tcp6 in v2.1.1, so every TCP series was duplicated.
    • P1 Correct the dae_dns_cache_hit_total help text. The counter includes lazy hits.
    • P1 Require endpoint_username and endpoint_password together. A password alone silently disabled auth.
    • P1 example.dae now binds the sample endpoint to 127.0.0.1.
    • P2 TLS file checks reject forbidden permission bits instead of requiring exact modes, so 0400 keys and 0600 certs pass. Remove the unused ValidateFilePermissionNotTooOpen.
    • P2 dae_health_check_total now counts only checks that reach a verdict.
  • ea9f706, 3006758 docs(metrics): replace the stale fork dashboards with the v6-based dashboard, which is also used in feat(metrics): add Prometheus /metrics endpoint with DNS, dialer, connection, and runtime stats daeuniverse/dae#1015
    • The network variable lists udp4(DNS)/udp6(DNS).
    • The success-rate panel shows No data when no checks ran.
    • The panel descriptions match the verdict-only counters.
  • 0fc3f93, d0eebc5, b0fd810 docs: the review report and its implementation status

Issue Reference

Follow-up to #38. Mirrors daeuniverse#1015 (5c9df67, 8502e56).

Test Result

Local checks used go1.26.0, the CI GOEXPERIMENT set, and -tags dae_stub_ebpf:

  • go build ./... and go vet pass.
  • golangci-lint v2.11.0 reports 0 issues. gofmt and go mod tidy are clean.
  • go test passes for ./pkg/... ./common/... ./component/outbound/... ./cmd ./config and for the metrics tests in ./control.
  • The new regression tests fail on the previous code and pass with this change:
    • TestPrometheusHandlerSurvivesCollectorError: status=500 before.
    • TestCheck_CountersCountOnlyVerdicts: total=3 before.

The full Linux runtime suites (go-test, bpf-test, kernel-test) run in CI on this PR.

🤖 Generated with Claude Code

https://claude.ai/code/session_01BYykYRdT6p8qLLe4RMiV7s


Generated by Claude Code

claude added 6 commits October 2, 2026 15:04
Review of the metrics code merged by PR #38, scoped to what is shared
with the upstream PR (40 files byte-identical to daeuniverse#1015 head 8573436).

Findings: node metrics export proxy credentials through the link label;
duplicate node names in a group fail the whole scrape with HTTP 500
(reproduced with client_golang v1.19.1); tcp4(DNS)/tcp6(DNS) series
alias tcp4/tcp6 under v2.1.1; plus HELP, BasicAuth, example bind
address, TLS permission and health-check counting fixes, each with a
minimal patch sketch and the rollout order for daeuniverse#1015 and fork main.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BYykYRdT6p8qLLe4RMiV7s
- Drop dae_node_latency_seconds and dae_node_alive: their link label is
  the node share link (Trojan password, SS cipher:password, VLESS/VMess
  ID), and they duplicate the per-dialer health metrics. Revert the
  NodeLatencySnapshot Name/Group fields they needed.
- Keep dialer label sets unique: same-named nodes in one group (common
  with several subscriptions) get a " #N" suffix. A duplicate label set
  made promhttp return HTTP 500 for the whole scrape; serve with
  ContinueOnError so one collector error cannot blank the endpoint.
- Export each distinct health collection once via StandardHealthKeys.
  tcp4(DNS)/tcp6(DNS) alias tcp4/tcp6 in v2.1.1 and doubled every TCP
  series. Remove the DialerGroup.AliveDialerSets accessor.
- Fix the dae_dns_cache_hit_total help: it includes lazy hits.
- Require endpoint_username and endpoint_password together; a password
  alone left the endpoint open.
- example.dae: bind the sample endpoint to 127.0.0.1.
- TLS files: reject group/other write on the certificate and any
  group/other access on the key instead of matching exact modes, so
  0400 keys and 0600 certificates pass. Remove the unused
  ValidateFilePermissionNotTooOpen.
- Count dae_health_check_total only for checks with a verdict; skips
  and probe-infrastructure failures diluted the failure ratio.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BYykYRdT6p8qLLe4RMiV7s
…hboard

Both fork dashboards queried the removed dae_node_* metrics and treated
dae_dns_cache_hit_total as fresh-only (hit + lazy double-counted stale
hits). Use the daeuniverse#1015 dashboard (blob 21ce121), which
derives fresh hits as hit - lazy and does not use dae_node_*.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BYykYRdT6p8qLLe4RMiV7s
Rename it back to "dae Transparent Proxy-Grafana_dashboard.json" and use
the same revision as daeuniverse#1015: the sanitized v6 export
(no id/version, datasource or group selection), with the network
variable listing udp4(DNS)/udp6(DNS), the health check success rate
showing No data when no checks ran, and the health check descriptions
matching the verdict-only counters.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BYykYRdT6p8qLLe4RMiV7s
@github-actions

github-actions Bot commented Oct 3, 2026

Copy link
Copy Markdown

DNS Benchmark Compare

  • Base ref: origin/main
  • Head ref: b0fd81039b1d85682ba6d367460fd949dc867101
  • Base strategy: merge-base
  • Suite profile: dns-module
  • Benchmark count: 3
  • Benchmark time: 200ms

Suite Status

  • control_dns_cache: success
  • component_upstream_hotpath: success

control_dns_cache

DNS Benchmark Compare

  • Base: origin/main (a25e50e5319300cbbca712e59565d965fb7629c8)
  • Head: b0fd81039b1d85682ba6d367460fd949dc867101 (b0fd81039b1d85682ba6d367460fd949dc867101)
  • Package: ./control
  • Benchmark filter: ^BenchmarkDnsCache_FillIntoWithTTL$
  • Common benchmarks: 1
  • Head-only benchmarks: 0

benchstat

goos: linux
goarch: amd64
pkg: github.com/daeuniverse/dae/control
cpu: AMD EPYC 9V74 80-Core Processor                
                           │ /home/runner/work/dae/dae/bench-artifacts/control_dns_cache/base_common.txt │ /home/runner/work/dae/dae/bench-artifacts/control_dns_cache/head_common.txt │
                           │                                   sec/op                                    │                          sec/op                           vs base           │
DnsCache_FillIntoWithTTL-4                                                                  501.8n ± ∞ ¹                                               504.2n ± ∞ ¹  ~ (p=1.000 n=3) ²
¹ need >= 6 samples for confidence interval at level 0.95
² need >= 4 samples to detect a difference at alpha level 0.05

                           │ /home/runner/work/dae/dae/bench-artifacts/control_dns_cache/base_common.txt │ /home/runner/work/dae/dae/bench-artifacts/control_dns_cache/head_common.txt │
                           │                                    B/op                                     │                           B/op                            vs base           │
DnsCache_FillIntoWithTTL-4                                                                   560.0 ± ∞ ¹                                                560.0 ± ∞ ¹  ~ (p=1.000 n=3) ²
¹ need >= 6 samples for confidence interval at level 0.95
² all samples are equal

                           │ /home/runner/work/dae/dae/bench-artifacts/control_dns_cache/base_common.txt │ /home/runner/work/dae/dae/bench-artifacts/control_dns_cache/head_common.txt │
                           │                                  allocs/op                                  │                        allocs/op                          vs base           │
DnsCache_FillIntoWithTTL-4                                                                   9.000 ± ∞ ¹                                                9.000 ± ∞ ¹  ~ (p=1.000 n=3) ²
¹ need >= 6 samples for confidence interval at level 0.95
² all samples are equal

component_upstream_hotpath

DNS Benchmark Compare

  • Base: origin/main (a25e50e5319300cbbca712e59565d965fb7629c8)
  • Head: b0fd81039b1d85682ba6d367460fd949dc867101 (b0fd81039b1d85682ba6d367460fd949dc867101)
  • Package: ./component/dns
  • Benchmark filter: ^BenchmarkUpstreamResolver_GetUpstream_(Serial|Parallel)$
  • Common benchmarks: 2
  • Head-only benchmarks: 0

benchstat

goos: linux
goarch: amd64
pkg: github.com/daeuniverse/dae/component/dns
cpu: AMD EPYC 9V74 80-Core Processor                
                                        │ /home/runner/work/dae/dae/bench-artifacts/component_upstream_hotpath/base_common.txt │ /home/runner/work/dae/dae/bench-artifacts/component_upstream_hotpath/head_common.txt │
                                        │                                        sec/op                                        │                            sec/op                             vs base                │
UpstreamResolver_GetUpstream_Serial-4                                                                             2.187n ± ∞ ¹                                                   2.191n ± ∞ ¹       ~ (p=1.000 n=3) ²
UpstreamResolver_GetUpstream_Parallel-4                                                                           5.355n ± ∞ ¹                                                   5.455n ± ∞ ¹       ~ (p=0.400 n=3) ²
geomean                                                                                                           3.422n                                                         3.457n        +1.02%
¹ need >= 6 samples for confidence interval at level 0.95
² need >= 4 samples to detect a difference at alpha level 0.05

                                        │ /home/runner/work/dae/dae/bench-artifacts/component_upstream_hotpath/base_common.txt │ /home/runner/work/dae/dae/bench-artifacts/component_upstream_hotpath/head_common.txt │
                                        │                                         B/op                                         │                             B/op                              vs base                │
UpstreamResolver_GetUpstream_Serial-4                                                                              0.000 ± ∞ ¹                                                    0.000 ± ∞ ¹       ~ (p=1.000 n=3) ²
UpstreamResolver_GetUpstream_Parallel-4                                                                            0.000 ± ∞ ¹                                                    0.000 ± ∞ ¹       ~ (p=1.000 n=3) ²
geomean                                                                                                                      ³                                                                 +0.00%               ³
¹ need >= 6 samples for confidence interval at level 0.95
² all samples are equal
³ summaries must be >0 to compute geomean

                                        │ /home/runner/work/dae/dae/bench-artifacts/component_upstream_hotpath/base_common.txt │ /home/runner/work/dae/dae/bench-artifacts/component_upstream_hotpath/head_common.txt │
                                        │                                      allocs/op                                       │                          allocs/op                            vs base                │
UpstreamResolver_GetUpstream_Serial-4                                                                              0.000 ± ∞ ¹                                                    0.000 ± ∞ ¹       ~ (p=1.000 n=3) ²
UpstreamResolver_GetUpstream_Parallel-4                                                                            0.000 ± ∞ ¹                                                    0.000 ± ∞ ¹       ~ (p=1.000 n=3) ²
geomean                                                                                                                      ³                                                                 +0.00%               ³
¹ need >= 6 samples for confidence interval at level 0.95
² all samples are equal
³ summaries must be >0 to compute geomean

@MaurUppi
MaurUppi merged commit 40dd125 into main Oct 3, 2026
44 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.

2 participants