Added missing OpenTelemetry instrumentation to several API endpoints. - #32960
Conversation
GitHub Actions
Inactive
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #32960 +/- ##
==========================================
+ Coverage 63.80% 63.82% +0.01%
==========================================
Files 2044 2045 +1
Lines 201333 201457 +124
Branches 6558 6558
==========================================
+ Hits 128460 128576 +116
- Misses 62711 62715 +4
- Partials 10162 10166 +4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
GitHub Actions
Inactive
GitHub Actions
Error
GitHub Actions
Inactive
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
WalkthroughAdds OpenTelemetry HTTP instrumentation across many endpoints by introducing OTEL middleware wrappers, applying them at route and mux levels, and propagating Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant C as Client
participant Root as rootMux
participant OT as OTEL Wrapper
participant H as Handler
Note over Root,OT: Route-level instrumentation (fixed-route)
C->>Root: HTTP Request (/healthz, /version, /mdm/..., /api/.../scim/...)
Root->>OT: Dispatch to wrapped handler (WrapHandler)
OT->>OT: Start span "{METHOD} {route}"
OT->>H: Invoke underlying handler
H-->>OT: Response / Error
OT-->>Root: End span
Root-->>C: HTTP Response
sequenceDiagram
autonumber
participant C as Client
participant Root as rootMux
participant OTd as OTEL Dynamic Wrapper
participant H as Dynamic Handler
Note over Root,OTd: Dynamic per-request instrumentation (assets, debug, dynamic proxies)
C->>Root: HTTP Request (/assets/app.js or /mdm/scep/proxy/...)
Root->>OTd: Create per-request instrumented handler using r.URL.Path
OTd->>OTd: Start span "{METHOD} {r.URL.Path}"
OTd->>H: Call handler
H-->>OTd: Response
OTd-->>Root: End span
Root-->>C: HTTP Response
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (10)
ee/server/service/hostidentity/scep.go (2)
103-104: Instrumented SCEP handler correctly, but consider route cardinality.WrapHandler uses a fixed route and span name, which is ideal. Keep this pattern for fixed paths elsewhere.
179-194: Double‑check public key type before ECDSA verification.If UnmarshalPublicKey can return non‑ECDSA keys, this will fail at compile time or panic at runtime. Add a type assertion and a clearer error.
Apply:
- pubKey, err := oldCertData.UnmarshalPublicKey() + pubKey, err := oldCertData.UnmarshalPublicKey() if err != nil { return nil, fmt.Errorf("unmarshaling public key: %w", err) } + ecdsaKey, ok := pubKey.(*ecdsa.PublicKey) + if !ok { + return nil, errors.New("unsupported public key type for renewal; expected ECDSA") + } @@ - if !ecdsa.VerifyASN1(pubKey, hash[:], sigBytes) { + if !ecdsa.VerifyASN1(ecdsaKey, hash[:], sigBytes) { return nil, errors.New("invalid renewal signature") }If UnmarshalPublicKey is guaranteed to return *ecdsa.PublicKey, please confirm and this change isn’t needed.
server/service/testing_utils.go (1)
507-517: Optional: mirror production by wrapping rootMux with an OTEL server handler in tests.If you want test traces to resemble prod (and to catch regressions in top‑level instrumentation), wrap rootMux when tracing is enabled.
Example (outside selected lines, for context):
if cfg.Logging.TracingEnabled && cfg.Logging.TracingType == "opentelemetry" { rootHandler := otelhttp.NewHandler(rootMux, "root") server := httptest.NewUnstartedServer(rootHandler) // ... } else { server := httptest.NewUnstartedServer(rootMux) }server/service/middleware/otel/otel.go (1)
29-45: Avoid high‑cardinality spans in WrapHandlerDynamic.Using r.URL.Path as the route (and span name) can explode cardinality (IDs, hashes, filenames). Prefer a normalized route label (e.g., “/assets/”, “/mdm/scep/proxy/”, “/Users/{id}”) and keep the raw path as a separate attribute if needed.
Apply:
-func WrapHandlerDynamic(handler http.Handler, config config.FleetConfig) http.Handler { +func WrapHandlerDynamic(handler http.Handler, config config.FleetConfig) http.Handler { if config.Logging.TracingEnabled && config.Logging.TracingType == "opentelemetry" { - // Create a wrapper that instruments each request with its actual path + // Create a wrapper that instruments each request with a normalized route return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - // Use the actual request path as the route - route := r.URL.Path + route := normalizeRoute(r.URL.Path) instrumentedHandler := otelhttp.NewHandler( - otelhttp.WithRouteTag(route, handler), + otelhttp.WithRouteTag(route, handler), "", // Empty operation name - will be set by span name formatter otelhttp.WithSpanNameFormatter(func(operation string, req *http.Request) string { - return req.Method + " " + route + return req.Method + " " + route }), ) instrumentedHandler.ServeHTTP(w, r) }) } return handler } + +// normalizeRoute collapses variable segments to keep http.route low-cardinality. +func normalizeRoute(path string) string { + switch { + case strings.HasPrefix(path, "/assets/"): + return "/assets/*" + case strings.HasPrefix(path, "/mdm/scep/proxy/"): + return "/mdm/scep/proxy/*" + case strings.HasPrefix(path, "/api/") && strings.Contains(path, "/fleet/scim/"): + // Basic SCIM normalization: /Users/{id}, /Groups/{id} + parts := strings.Split(path, "/") + if n := len(parts); n >= 1 { + // find trailing ID-like segment and replace by {id} + if n > 0 { + last := parts[n-1] + if last != "" && last != "Users" && last != "Groups" { + parts[n-1] = "{id}" + } + } + return strings.Join(parts, "/") + } + } + return path +}If you prefer flexibility, expose a WrapHandlerWithResolver(handler, config, func(*http.Request) string) variant and keep WrapHandlerDynamic as-is.
ee/server/scim/scim.go (1)
219-229: OTEL added to SCIM chain, but consider normalized routes.WrapHandlerDynamic will currently use the raw (post‑StripPrefix) path. For /Users/{id} and /Groups/{id}, that creates high‑cardinality routes. Either adopt a SCIM‑specific normalizer here or switch to a resolver‑based wrapper (see suggested otel middleware refactor).
Apply (after adding normalizeRoute as suggested in otel middleware):
- handler = otel.WrapHandlerDynamic(handler, *fleetConfig) + handler = otel.WrapHandlerDynamic(handler, *fleetConfig) // consider normalized resolverOptionally, replace with a SCIM resolver:
handler = otel.WrapHandlerWithResolver(handler, *fleetConfig, func(r *http.Request) string { p := r.URL.Path if strings.HasPrefix(p, "/Users/") { return "/Users/{id}" } if strings.HasPrefix(p, "/Groups/") { return "/Groups/{id}" } return p })server/service/handler.go (2)
1269-1271: SCEP proxy uses dynamic wrapper — watch span cardinality.The proxy path likely includes variable segments. Consider the normalized route approach suggested for WrapHandlerDynamic to avoid high‑cardinality metrics/spans.
244-248: Note for future changes: keep an eye on SQL in new endpoints.No SQL added here, but per guidelines ensure any future SQL in these handlers includes precise filters (WHERE clauses) to avoid unintended reads.
cmd/fleet/serve.go (3)
1238-1241: Wrap static and simple endpoints with fixed, low‑cardinality route tags.
Dynamic wrapping for/assets/can explodehttp.routecardinality (per‑file). Prefer a fixed pattern.Apply this diff:
- rootMux.Handle("/assets/", service.PrometheusMetricsHandler("static_assets", otelmw.WrapHandlerDynamic(service.ServeStaticAssets("/assets/"), config))) + rootMux.Handle("/assets/", service.PrometheusMetricsHandler( + "static_assets", + otelmw.WrapHandler(service.ServeStaticAssets("/assets/"), "/assets/*", config), + ))Also, please confirm that
/healthzand/versionspans use the exact canonical values"/healthz"and"/version"forhttp.route, and that/api/*endpoints are instrumented withinservice.MakeHandler(...)(or at another layer) since they are not wrapped here.
1313-1319: Don’t trace /metrics to avoid noisy spans and scrape amplification.
Prometheus scrapes can generate a lot of low‑value spans. Consider leaving/metricsuninstrumented.Apply this diff in both branches:
- service.PrometheusMetricsHandler("metrics", otelmw.WrapHandler(promhttp.Handler(), "/metrics", config)) + service.PrometheusMetricsHandler("metrics", promhttp.Handler())Also applies to: 1320-1323
1440-1441: Frontend catch‑all: avoid high‑cardinalityhttp.route.
Dynamic wrapping of/can emit one route value per UI path. Use a fixed label.- rootMux.Handle("/", otelmw.WrapHandlerDynamic(frontendHandler, config)) + rootMux.Handle("/", otelmw.WrapHandler(frontendHandler, "/*", config))
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
changes/32331-otel-instrumentation(1 hunks)cmd/fleet/serve.go(6 hunks)ee/server/scim/scim.go(5 hunks)ee/server/service/hostidentity/scep.go(3 hunks)server/service/handler.go(11 hunks)server/service/middleware/otel/otel.go(1 hunks)server/service/testing_utils.go(4 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
⚙️ CodeRabbit configuration file
When reviewing SQL queries that are added or modified, ensure that appropriate filtering criteria are applied—especially when a query is intended to return data for a specific entity (e.g., a single host). Check for missing WHERE clauses or incorrect filtering that could lead to incorrect or non-deterministic results (e.g., returning the first row instead of the correct one). Flag any queries that may return unintended results due to lack of precise scoping.
Files:
server/service/middleware/otel/otel.goee/server/service/hostidentity/scep.goee/server/scim/scim.goserver/service/handler.goserver/service/testing_utils.gocmd/fleet/serve.go
🧠 Learnings (1)
📚 Learning: 2025-08-08T08:32:31.529Z
Learnt from: getvictor
PR: fleetdm/fleet#31695
File: server/datastore/mysql/apple_mdm_test.go:132-132
Timestamp: 2025-08-08T08:32:31.529Z
Learning: Datastore.NewMDMWindowsConfigProfile signature is: NewMDMWindowsConfigProfile(ctx context.Context, cp fleet.MDMWindowsConfigProfile, usesFleetVars []string) (*fleet.MDMWindowsConfigProfile, error). Passing nil for usesFleetVars in tests denotes “no Fleet variables referenced” and is used consistently across the repo.
Applied to files:
server/service/handler.go
🔇 Additional comments (18)
changes/32331-otel-instrumentation (1)
1-1: Changelog entry LGTM.Reads clearly and matches the PR scope.
ee/server/service/hostidentity/scep.go (1)
71-75: Good guard: nil fleetConfig.Prevents a panic when dereferencing the pointer later.
server/service/testing_utils.go (4)
445-445: Signature update wired correctly.Passing cfg by value aligns with the new RegisterAppleMDMProtocolServices signature.
463-463: SCEP proxy registration updated.Pointer form matches the new API and enables OTEL wrappers.
484-489: Host Identity SCEP + HTTP Sig middleware wiring looks good.SCEP now receives cfg and is instrumented; HTTP message signature verifier remains optional via opts.
501-505: SCIM registration matches new API.Config pointer provided; details endpoints routed to the same apiHandler as before.
server/service/middleware/otel/otel.go (1)
12-24: Fixed‑route wrapper is solid and spec‑friendly.Using WithRouteTag + SpanName "{METHOD} {route}" avoids high cardinality and matches HTTP semconv guidance.
ee/server/scim/scim.go (3)
35-39: Good: enforce non‑nil fleetConfig.Prevents nil deref in the OTEL wrapper path.
264-274: SCIM error parsing/readback looks correct.Covers InvalidValue case and customizes details for /Users as intended. Bound length to SCIMMaxFieldLength is a good guard.
305-321: Switch to scimerrors.ScimError is fine.JSON encoding and content type are correct; logs on marshal/write failure are appropriate.
server/service/handler.go (6)
1176-1186: Apple MDM protocol services now receive FleetConfig; good propagation.All three internal registrations (SCEP, MDM, service discovery) accept and use the config to drive OTEL wrappers.
1194-1209: Service discovery instrumentation OK.Fixed route label prevents cardinality issues; keep this pattern for other fixed endpoints.
1245-1246: SCEP handler: fixed‑route OTEL wrapper LGTM.
1254-1258: Good: nil check for fleetConfig in SCEP proxy registration.Prevents nil deref down the line.
1340-1341: MDM handler wrapped with fixed route — LGTM.
120-132: Gorilla/mux OTEL middleware span naming matches spec.Using "{METHOD} {route}" is consistent with HTTP semconv. APM fallback preserved.
cmd/fleet/serve.go (2)
64-64: OTEL middleware import wired correctly.
Import alias looks good and consistent with usage below.
1285-1285: Incorrect — do not change to &config; the function expects config.MDMConfig (by value).
Signature: func RegisterAppleMDMProtocolServices(..., scepConfig config.MDMConfig) — server/service/handler.go:1166-1168. The call passing config.MDM in cmd/fleet/serve.go:1275-1277 is correct.Likely an incorrect or invalid review comment.
| } | ||
| rootMux.Handle("/debug/", debugHandler) | ||
| rootMux.Handle("/debug/", otelmw.WrapHandlerDynamic(debugHandler, config)) | ||
|
|
There was a problem hiding this comment.
Potential token leak in traces from /debug/?token=… — disable or scrub query.
Instrumenting /debug/ may capture the token query param in span attributes. Either skip tracing for /debug/ or ensure query strings are stripped/scrubbed.
- rootMux.Handle("/debug/", otelmw.WrapHandlerDynamic(debugHandler, config))
+ // Avoid tracing sensitive debug endpoints (token in query)
+ rootMux.Handle("/debug/", debugHandler)If you must keep traces, ensure the middleware drops query strings for this path.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Avoid tracing sensitive debug endpoints (token in query) | |
| rootMux.Handle("/debug/", debugHandler) |
There was a problem hiding this comment.
The token is not intended for production. The token is only generated and displayed when the --debug flag is used, which should only be in development environments. The token provides a simpler way to access debugging tools like pprof, expvar, and other netbug endpoints during development.
GitHub Actions
Inactive
GitHub Actions
Inactive
GitHub Actions
Error
GitHub Actions
Inactive
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
cmd/fleet/serve.go (1)
1447-1447: Dynamic tracing on /debug — acknowledged risk.Prior note about query token; keeping dynamic wrapper in dev is acceptable.
🧹 Nitpick comments (9)
server/service/middleware/otel/otel.go (3)
12-24: Avoid parameter shadowing; use cfg instead of config.Parameter name shadows the imported package name, which hurts readability.
-func WrapHandler(handler http.Handler, route string, config config.FleetConfig) http.Handler { - if config.Logging.TracingEnabled && config.Logging.TracingType == "opentelemetry" { +func WrapHandler(handler http.Handler, route string, cfg config.FleetConfig) http.Handler { + if cfg.Logging.TracingEnabled && cfg.Logging.TracingType == "opentelemetry" {
26-45: Same shadowing nit in WrapHandlerDynamic.-func WrapHandlerDynamic(handler http.Handler, config config.FleetConfig) http.Handler { - if config.Logging.TracingEnabled && config.Logging.TracingType == "opentelemetry" { +func WrapHandlerDynamic(handler http.Handler, cfg config.FleetConfig) http.Handler { + if cfg.Logging.TracingEnabled && cfg.Logging.TracingType == "opentelemetry" {
26-45: High-cardinality risk with dynamic route tags.Using r.URL.Path as the route tag can explode span cardinality (e.g., hashed asset names, IDs). Prefer passing a pattern (e.g., “/assets/*”) via WrapHandler where possible, or sanitize dynamic paths.
Do you want a small sanitizer (collapse long hex/UUID/numeric segments to “:id”) added here?
server/contexts/ctxerr/ctxerr.go (2)
323-324: Redundant nil check on Span.trace.SpanFromContext returns a non-nil Span; IsRecording is sufficient.
- if span := trace.SpanFromContext(ctx); span != nil && span.IsRecording() { + if span := trace.SpanFromContext(ctx); span.IsRecording() {
322-352: Attribute size considerations for exception.stacktrace.Large stacktraces may be truncated by backends. Consider bounding length or attaching as an event attribute only when needed.
server/contexts/ctxerr/ctxerr_otel_test.go (1)
84-85: Remove double span.End().span.End() is called here and also via defer previously; keep only one.
- span.End() + span.End()Note: after applying the prior diff, ensure only a single End() remains.
changes/32331-otel-instrumentation (1)
1-1: Changelog entry is too terse; list endpoints and gating.Add instrumented routes and the config gate so operators know when spans appear.
-* Added missing OpenTelemetry instrumentation to several API endpoints. +* Added OpenTelemetry instrumentation to API endpoints: + - /healthz, /version, /assets/*, /enroll, / (UI), /metrics, /debug/*, MDM/SCIM/SCEP routes. +* Span names use "{METHOD} {route}" and set http.route. +* Enabled when logging.tracing_enabled=true and logging.tracing_type="opentelemetry".ee/server/scim/scim_otel_test.go (1)
150-152: Remove duplicate span.End().Span is already deferred; calling End twice is redundant.
- // Force span to end - span.End() + // End handled by defercmd/fleet/serve.go (1)
1238-1241: Instrument health/version; avoid high-cardinality on /assets.Health/version OK. Prefer fixed route for assets to reduce cardinality.
-rootMux.Handle("/assets/", service.PrometheusMetricsHandler("static_assets", otelmw.WrapHandlerDynamic(service.ServeStaticAssets("/assets/"), config))) +rootMux.Handle("/assets/", service.PrometheusMetricsHandler("static_assets", otelmw.WrapHandler(service.ServeStaticAssets("/assets/"), "/assets/*", config)))
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
changes/32331-otel-instrumentation(1 hunks)cmd/fleet/serve.go(6 hunks)ee/server/scim/scim.go(6 hunks)ee/server/scim/scim_otel_test.go(1 hunks)ee/server/service/hostidentity/scep.go(3 hunks)server/contexts/ctxerr/ctxerr.go(2 hunks)server/contexts/ctxerr/ctxerr_otel_test.go(1 hunks)server/service/handler.go(11 hunks)server/service/middleware/otel/otel.go(1 hunks)server/service/testing_utils.go(4 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
⚙️ CodeRabbit configuration file
When reviewing SQL queries that are added or modified, ensure that appropriate filtering criteria are applied—especially when a query is intended to return data for a specific entity (e.g., a single host). Check for missing WHERE clauses or incorrect filtering that could lead to incorrect or non-deterministic results (e.g., returning the first row instead of the correct one). Flag any queries that may return unintended results due to lack of precise scoping.
Files:
server/service/middleware/otel/otel.goserver/contexts/ctxerr/ctxerr.goee/server/scim/scim.goee/server/service/hostidentity/scep.goserver/contexts/ctxerr/ctxerr_otel_test.goserver/service/handler.goee/server/scim/scim_otel_test.goserver/service/testing_utils.gocmd/fleet/serve.go
🧠 Learnings (1)
📚 Learning: 2025-08-08T08:32:31.529Z
Learnt from: getvictor
PR: fleetdm/fleet#31695
File: server/datastore/mysql/apple_mdm_test.go:132-132
Timestamp: 2025-08-08T08:32:31.529Z
Learning: Datastore.NewMDMWindowsConfigProfile signature is: NewMDMWindowsConfigProfile(ctx context.Context, cp fleet.MDMWindowsConfigProfile, usesFleetVars []string) (*fleet.MDMWindowsConfigProfile, error). Passing nil for usesFleetVars in tests denotes “no Fleet variables referenced” and is used consistently across the repo.
Applied to files:
server/service/handler.go
🔇 Additional comments (17)
server/contexts/ctxerr/ctxerr.go (1)
299-301: Early return on nil error — good guard.ee/server/service/hostidentity/scep.go (1)
71-76: SCEP handler OTEL-wrapped with route tag — LGTM.Nil-guard for fleetConfig and WrapHandler usage are correct.
Ensure the SCEP route isn’t also wrapped at a higher mux level to avoid duplicate server spans.
Also applies to: 103-104
server/service/testing_utils.go (1)
444-445: Approve: FleetConfig callsites updated and consistent.
Signatures match; by-value vs pointer usage (cfg.MDM vs &cfg) is correct across callsites.server/service/middleware/otel/otel.go (1)
10-24: No double-instrumentation detected — per-route instrumentation only.Repo search shows handlers are wrapped per-route (otelmw.WrapHandler / WrapHandlerDynamic in cmd/fleet/serve.go and many handler registrations) and otelhttp.NewHandler is only used on individual handlers (server/service/middleware/otel/otel.go, ee/server/scim/scim.go); no otelhttp.NewHandler(rootMux, ...) or global/rootMux-level OTEL wrapping was found.
ee/server/scim/scim.go (3)
219-235: Middleware order looks correct; LGTM.SetRequestsContext → logging → last‑request → authn → authz → SCIM handler is sensible.
382-399: SCIM error handling migration; LGTM.Switch to scimerrors preserves status/detail encoding and content‑type.
304-311: otelhttp.NewHandler misuse (compile-time bug).First arg must be http.Handler; WithRouteTag returns an Option. This won’t compile.
- // Create the instrumented handler with the proper route - instrumentedHandler := otelhttp.NewHandler( - otelhttp.WithRouteTag(route, next), - "", // Empty operation name - will be set by span name formatter - otelhttp.WithSpanNameFormatter(func(operation string, req *http.Request) string { - return req.Method + " " + route - }), - ) + // Create the instrumented handler with the proper route + instrumentedHandler := otelhttp.NewHandler( + next, + "", + otelhttp.WithRouteTag(route), + otelhttp.WithSpanNameFormatter(func(operation string, req *http.Request) string { + return req.Method + " " + route + }), + )Likely an incorrect or invalid review comment.
server/service/handler.go (6)
1194-1210: Service discovery wrapped with fixed route; LGTM.Uses fixed apple_mdm.ServiceDiscoveryPath to keep http.route low‑cardinality.
1220-1246: SCEP handler wrapped with fixed route; LGTM.Good choice to avoid leaking dynamic path segments.
1254-1258: Nil guard for FleetConfig; LGTM.Avoids hidden panics during instrumentation.
1269-1271: Use fixed-route wrapper for SCEP proxy; LGTM.Prevents exposing {identifier} in span names/route tags as intended.
1310-1342: MDM handler instrumentation via fixed route; LGTM.Consistent, low‑cardinality http.route tagging.
1176-1187: Propagate FleetConfig through Apple MDM registrations — update all call sitesRegisterAppleMDMProtocolServices now accepts a fleetConfig parameter; update callers to pass the FleetConfig (e.g., cfg.Fleet / config.Fleet). Call sites found: server/service/testing_utils.go:431, cmd/fleet/serve.go:1275.
cmd/fleet/serve.go (4)
64-65: Otel middleware import; LGTM.
1294-1297: Updated RegisterSCEPProxy/RegisterSCIM call sites; LGTM.
1318-1323: /metrics instrumentation; LGTM.Consistent wrapper in both auth and no‑auth cases.
1441-1442: Frontend and enroll instrumentation; LGTM.
GitHub Actions
Error
GitHub Actions
Inactive
| var route string | ||
|
|
||
| // Debug: Log the actual path we're processing | ||
| // fmt.Printf("DEBUG: SCIM OTEL fullPath=%q, scimPath=%q\n", fullPath, scimPath) |
There was a problem hiding this comment.
Remove this debug comment? Or do you want to leave it in case you need it in the future
There was a problem hiding this comment.
Good catch. I will remove it in the next OTEL PR.
3 participants
Fixes #32331
Manually tested all paths.
/testpath removed in #32962Also added support for sending errors to OpenTelemetry, like we do for APM/Sentry.
Checklist for submitter
changes/,orbit/changes/oree/fleetd-chrome/changes.See Changes files for more information.
Testing
Summary by CodeRabbit
New Features
Tests