-
Notifications
You must be signed in to change notification settings - Fork 0
fix(runner/jaeger): fan out SPM metrics to match the backend composer #528
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,6 +2,7 @@ package jaeger | |
|
|
||
| import ( | ||
| "context" | ||
| "encoding/json" | ||
| "net/http" | ||
| "net/http/httptest" | ||
| "strings" | ||
|
Comment on lines
4
to
8
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
|
|
@@ -95,25 +96,74 @@ func TestOperations_BuildsPath(t *testing.T) { | |
| } | ||
| } | ||
|
|
||
| func TestMetrics_RequiresType(t *testing.T) { | ||
| c := New("http://x", nil) | ||
| if _, err := c.Metrics(context.Background(), "", map[string]any{}); err == nil { | ||
| t.Error("expected error") | ||
| // TestMetrics_FansOutAndRemaps verifies the SPM fan-out: four endpoints are | ||
| // hit (calls, errors, latencies@0.95, latencies@0.99), the composer's plural | ||
| // services/spanKinds are remapped to singular service/spanKind, metric_type is | ||
| // dropped, and the responses are assembled under the expected keys. | ||
| func TestMetrics_FansOutAndRemaps(t *testing.T) { | ||
| hits := map[string]string{} // metric path -> raw query | ||
| c, srv := newTestClient(func(w http.ResponseWriter, r *http.Request) { | ||
| hits[r.URL.Path] = r.URL.RawQuery | ||
| // Echo the metric name so we can assert the assembly mapping. | ||
| _, _ = w.Write([]byte(`{"path":"` + r.URL.Path + `"}`)) | ||
| }) | ||
| defer srv.Close() | ||
|
|
||
| raw, err := c.Metrics(context.Background(), map[string]any{ | ||
| "services": "frontend", | ||
| "spanKinds": "SPAN_KIND_SERVER", | ||
| "metric_type": "should-be-dropped", | ||
| "endTs": 1700000000, | ||
| }) | ||
| if err != nil { | ||
| t.Fatal(err) | ||
| } | ||
|
|
||
| // All four endpoints hit. | ||
| for _, p := range []string{"/api/metrics/calls", "/api/metrics/errors", "/api/metrics/latencies"} { | ||
| if _, ok := hits[p]; !ok { | ||
| t.Errorf("missing request to %s (hits: %v)", p, hits) | ||
| } | ||
| } | ||
| // Remap: singular service/spanKind present, plurals + metric_type gone. | ||
| q := hits["/api/metrics/calls"] | ||
| if !strings.Contains(q, "service=frontend") || !strings.Contains(q, "spanKind=SPAN_KIND_SERVER") { | ||
| t.Errorf("calls query = %q; want remapped service/spanKind", q) | ||
| } | ||
| if strings.Contains(q, "services=") || strings.Contains(q, "spanKinds=") || strings.Contains(q, "metric_type=") { | ||
| t.Errorf("calls query = %q; plural/metric_type should be dropped", q) | ||
| } | ||
| if !strings.Contains(q, "endTs=1700000000") { | ||
| t.Errorf("calls query = %q; want endTs passed through", q) | ||
| } | ||
|
|
||
| // Assembled shape. | ||
| var out map[string]json.RawMessage | ||
| if err := json.Unmarshal(raw, &out); err != nil { | ||
| t.Fatalf("assembled result not JSON object: %v", err) | ||
| } | ||
| for _, k := range []string{"calls", "errors", "latencies_p95", "latencies_p99"} { | ||
| if _, ok := out[k]; !ok { | ||
| t.Errorf("assembled result missing key %q (got %v)", k, out) | ||
| } | ||
| } | ||
| // The two latency sub-queries carry the right quantiles. | ||
| if lat := hits["/api/metrics/latencies"]; !strings.Contains(lat, "quantile=0.9") { | ||
| t.Errorf("latencies query = %q; want a quantile", lat) | ||
| } | ||
| } | ||
|
Comment on lines
+103
to
154
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The test Protecting the map with a func TestMetrics_FansOutAndRemaps(t *testing.T) {
var mu sync.Mutex
hits := map[string][]string{} // metric path -> raw queries
c, srv := newTestClient(func(w http.ResponseWriter, r *http.Request) {
mu.Lock()
hits[r.URL.Path] = append(hits[r.URL.Path], r.URL.RawQuery)
mu.Unlock()
// Echo the metric name so we can assert the assembly mapping.
_, _ = w.Write([]byte("{\"path\":\"" + r.URL.Path + "\"}"))
})
defer srv.Close()
raw, err := c.Metrics(context.Background(), map[string]any{
"services": "frontend",
"spanKinds": "SPAN_KIND_SERVER",
"metric_type": "should-be-dropped",
"endTs": 1700000000,
})
if err != nil {
t.Fatal(err)
}
// All four endpoints hit.
mu.Lock()
defer mu.Unlock()
for _, p := range []string{"/api/metrics/calls", "/api/metrics/errors", "/api/metrics/latencies"} {
if _, ok := hits[p]; !ok {
t.Errorf("missing request to %s (hits: %v)", p, hits)
}
}
// Remap: singular service/spanKind present, plurals + metric_type gone.
callsQueries := hits["/api/metrics/calls"]
if len(callsQueries) == 0 {
t.Fatal("missing calls query")
}
q := callsQueries[0]
if !strings.Contains(q, "service=frontend") || !strings.Contains(q, "spanKind=SPAN_KIND_SERVER") {
t.Errorf("calls query = %q; want remapped service/spanKind", q)
}
if strings.Contains(q, "services=") || strings.Contains(q, "spanKinds=") || strings.Contains(q, "metric_type=") {
t.Errorf("calls query = %q; plural/metric_type should be dropped", q)
}
if !strings.Contains(q, "endTs=1700000000") {
t.Errorf("calls query = %q; want endTs passed through", q)
}
// Assembled shape.
var out map[string]json.RawMessage
if err := json.Unmarshal(raw, &out); err != nil {
t.Fatalf("assembled result not JSON object: %v", err)
}
for _, k := range []string{\"calls\", \"errors\", \"latencies_p95\", \"latencies_p99\"} {
if _, ok := out[k]; !ok {
t.Errorf("assembled result missing key %q (got %v)", k, out)
}
}
// The two latency sub-queries carry the right quantiles.
latencies := hits["/api/metrics/latencies"]
if len(latencies) != 2 {
t.Errorf("expected 2 latency queries, got %d", len(latencies))
}
for _, lat := range latencies {
if !strings.Contains(lat, "quantile=0.9") {
t.Errorf("latencies query = %q; want a quantile", lat)
}
}
} |
||
|
|
||
| func TestMetrics_BuildsPath(t *testing.T) { | ||
| var path string | ||
| // TestMetrics_SPMNotAvailable: a 404 from Jaeger (SPM storage not wired up) | ||
| // surfaces the friendly legacy message, not a raw HTTP 404. | ||
| func TestMetrics_SPMNotAvailable(t *testing.T) { | ||
| c, srv := newTestClient(func(w http.ResponseWriter, r *http.Request) { | ||
| path = r.URL.RequestURI() | ||
| _, _ = w.Write([]byte(`{}`)) | ||
| w.WriteHeader(http.StatusNotFound) | ||
| _, _ = w.Write([]byte(`404 page not found`)) | ||
| }) | ||
| defer srv.Close() | ||
| if _, err := c.Metrics(context.Background(), "latencies", map[string]any{"service": "frontend"}); err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| if !strings.HasPrefix(path, "/api/metrics/latencies") { | ||
| t.Errorf("path = %q", path) | ||
| _, err := c.Metrics(context.Background(), map[string]any{"services": "frontend"}) | ||
| if err == nil || !strings.Contains(err.Error(), "SPM metrics not available") { | ||
| t.Errorf("expected friendly SPM-unavailable error, got %v", err) | ||
| } | ||
| } | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current implementation of
Metricsqueries the four SPM endpoints sequentially. Since each query is an independent HTTP request, executing them sequentially can significantly increase the total latency of theMetricscall (up to the sum of all four requests). Fanning out these requests concurrently using goroutines and a buffered channel will reduce the total latency to the maximum latency of a single request, greatly improving performance.