Skip to content

feat(observability): add semantic pkgsite spans - #12

Merged
garrettladley merged 1 commit into
mainfrom
gml/revamp-observability
May 23, 2026
Merged

garrettladley merged 1 commit into
mainfrom
gml/revamp-observability

Conversation

@garrettladley

Copy link
Copy Markdown
Owner
  • centralize pkgsite span attributes behind typed helpers in internal/observability
  • add semantic tool, cache, envelope, warmer, and rate-limit attributes for higher-signal sentry traces
  • record cache endpoint, outcome, ttl, status, and write outcome without exposing raw urls or client ips
  • notify sentry deployments from .github/workflows/fly-deploy.yml using the stamped release value

@garrettladley
garrettladley enabled auto-merge (squash) May 23, 2026 17:25
@garrettladley
garrettladley merged commit 45cb2b4 into main May 23, 2026
3 checks passed
@garrettladley
garrettladley deleted the gml/revamp-observability branch May 23, 2026 17:25
@greptile-apps

greptile-apps Bot commented May 23, 2026

Copy link
Copy Markdown

Greptile Summary

This PR centralizes all OpenTelemetry span attributes behind typed helper structs in a new internal/observability/attrs.go file, replacing scattered attribute.String(...) calls across the cache transport, warmer, rate-limit middleware, and tool handlers. It also wires Sentry deployment notifications into the Fly deploy workflow.

  • New attrs.go introduces typed structs (ToolAttrs, CacheLookupAttrs, WarmAttrs, RateLimitAttrs, EnvelopeAttrs, ResultAttrs) with Attributes() methods, plus helpers like ClassifyVersion, EndpointFromURL, and RemainingBucket for bucketed telemetry values.
  • AsyncWarmer.run signature changes to return (WarmOutcome, error), enabling per-job outcome spans with finer-grained outcome states.
  • Sentry step in fly-deploy.yml notifies Sentry of each production deploy using the stamped sha-<12> release value; fetch-depth is widened to 0 to supply the full commit history.

Confidence Score: 4/5

Safe to merge; changes are limited to telemetry instrumentation and a CI workflow step with no impact on business logic.

The core logic changes are well-structured and the tests follow the changed paths. Two small issues exist: ClassifyVersion uses the raw untrimmed version string in its default branch, and intArg relies on an int type assertion that would silently return 0 if the MCP SDK deserializes JSON numbers as float64. Neither affects request handling, only what gets recorded in traces.

internal/observability/attrs.go (ClassifyVersion whitespace handling) and internal/mcpserver/tools/register.go (intArg type assertion) are worth a second look before merging.

Important Files Changed

Filename Overview
internal/observability/attrs.go New file centralizing all span attribute helpers; contains a whitespace-handling inconsistency in ClassifyVersion's default branch.
internal/observability/attrs_test.go New test file covering EndpointFromURL, ClassifyVersion, and RemainingBucket; missing vulns endpoint test cases.
internal/pkgsite/transport/cache.go Cache transport migrated to typed CacheLookupAttrs; adds error recording for upstream client failures and write-outcome tracking.
internal/pkgsite/warmer.go AsyncWarmer.run now returns a WarmOutcome alongside the error; adds per-job spans with outcome attributes and a validWarmKind guard.
internal/mcpserver/tools/register.go Tool instrumentation migrated to typed ToolAttrs/EnvelopeAttrs; intArg may silently return 0 for JSON-decoded float64 limits.
internal/middleware/ratelimit.go Rate-limit middleware now records outcome, remaining bucket, limit, and window on each span branch.
.github/workflows/fly-deploy.yml Adds Sentry deployment notification step; fetch-depth changed to 0 to provide commit history.
internal/mcpserver/tools/tools_explain.go Explain tool migrated from raw attribute.String calls to ToolAttrs struct; no logic changes.
internal/observability/cache.go RecordCacheLookup and RecordCacheWrite updated to use centralized attribute constants and CacheWriteOutcomeFromOK helper.
internal/pkgsite/client_test.go Test updated for the new (WarmOutcome, error) return signature of AsyncWarmer.run.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[HTTP Request] --> B{CachedDoer.Do}
    B -->|non-GET or disabled| C[RecordCacheLookup: bypass/disabled]
    B -->|GET + enabled| D[Start pkgsite.cache lookup span]
    D --> E{kv.Store.Get}
    E -->|hit| F[outcome=hit]
    E -->|error| G[outcome=error, RecordError]
    E -->|miss| H[outcome=miss]
    G --> I[d.client.Do upstream]
    H --> I
    I -->|error| J[RecordError + return]
    I -->|ok| K{cacheTTL > 0?}
    K -->|yes| L[store.Set, writeOutcome=ok/error]
    K -->|no| M[writeOutcome=skipped]
    L --> N[SetAttributes: ttl, statusCode, writeOutcome]
    M --> N
    N --> O[Return response]
Loading

Comments Outside Diff (1)

  1. internal/mcpserver/tools/register.go, line 135-141 (link)

    P2 intArg uses a direct int type assertion. Go's encoding/json unmarshals all JSON numbers into float64 when decoding into any, so if nextArgs originates from a JSON-decoded payload, args[key].(int) will always fail silently and return 0, meaning pkgsite.query.limit would never be recorded in traces.

Reviews (1): Last reviewed commit: "feat(observability): add semantic pkgsit..." | Re-trigger Greptile

Comment on lines +310 to +324
func ClassifyVersion(version string) VersionClass {
switch strings.TrimSpace(version) {
case "":
return VersionClassEmpty
case "latest":
return VersionClassFloating
case "main", "master":
return VersionClassBranch
default:
if strings.HasPrefix(version, "v") && strings.Contains(version, ".") {
return VersionClassPinned
}
return VersionClassOther
}
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 ClassifyVersion trims whitespace before the switch but then uses the raw, untrimmed version inside the default case. A caller passing " v1.2.3" would fall through the switch correctly but then fail the HasPrefix("v") guard on the original string and be misclassified as VersionClassOther instead of VersionClassPinned.

Suggested change
func ClassifyVersion(version string) VersionClass {
switch strings.TrimSpace(version) {
case "":
return VersionClassEmpty
case "latest":
return VersionClassFloating
case "main", "master":
return VersionClassBranch
default:
if strings.HasPrefix(version, "v") && strings.Contains(version, ".") {
return VersionClassPinned
}
return VersionClassOther
}
}
func ClassifyVersion(version string) VersionClass {
version = strings.TrimSpace(version)
switch version {
case "":
return VersionClassEmpty
case "latest":
return VersionClassFloating
case "main", "master":
return VersionClassBranch
default:
if strings.HasPrefix(version, "v") && strings.Contains(version, ".") {
return VersionClassPinned
}
return VersionClassOther
}
}

Comment on lines +21 to +22
{raw: "https://pkg.go.dev/v1beta/imported-by/golang.org%2Fx%2Foauth2", want: PkgsiteEndpointImportedBy},
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 TestEndpointFromURL covers every endpoint except PkgsiteEndpointVulns, which has a uniquely branched match — EndpointFromURL accepts both vuln/ and vulns/ prefixes. Without a test case here, neither path is exercised.

Suggested change
{raw: "https://pkg.go.dev/v1beta/imported-by/golang.org%2Fx%2Foauth2", want: PkgsiteEndpointImportedBy},
}
{raw: "https://pkg.go.dev/v1beta/imported-by/golang.org%2Fx%2Foauth2", want: PkgsiteEndpointImportedBy},
{raw: "https://pkg.go.dev/v1beta/vuln/GO-2021-0001", want: PkgsiteEndpointVulns},
{raw: "https://pkg.go.dev/v1beta/vulns/GO-2021-0001", want: PkgsiteEndpointVulns},
}

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.

1 participant