From 49eba8b15bcfc07eb272f23f1b6810d37e6fe342 Mon Sep 17 00:00:00 2001 From: Michael Anthony Knyszek Date: Tue, 28 Jan 2025 20:54:34 +0000 Subject: [PATCH] internal/trace: interpret string ID arguments for experimental events Currently one of the reasons experimental events are tricky to use is because: - There's no way to take advantage of the existing infrastructure, like strings and stacks, and - There's no way to attach arbitrary data to an event (except through strings, possibly). Fix this by abstracting away the raw arguments in an ExperimentalEvent and requiring access to the arguments via a new method, ArgValue. This returns a Value, which gives us an opportunity to construct a typed value for the raw argument dynamically, and a way to access existing tables. The type of the argument is deduced from conventions for the argument's name. This seems more than sufficient for experimental events. To make this work, we also need to add a "string" variant to the Value type. This may be a little confusing since they're primarily used for metrics, but one could imagine other scenarios in which this is useful, such as including build information in the trace as a metric, so I think this is fine. This change also updates the Value API to accomodate a String method for use with things that expect a fmt.Stringer, which means renaming the value assertion methods to have a "To" prefix. Change-Id: I43a2334f6cd306122c5b94641a6252ca4258b39f Reviewed-on: https://go-review.googlesource.com/c/go/+/645135 Reviewed-by: Michael Pratt LUCI-TryBot-Result: Go LUCI Auto-Submit: Michael Knyszek --- src/cmd/trace/gen.go | 6 +-- src/internal/trace/event.go | 51 +++++++++++++++------- src/internal/trace/gc.go | 2 +- src/internal/trace/testtrace/validation.go | 2 +- src/internal/trace/tracev2/spec.go | 4 +- src/internal/trace/value.go | 48 ++++++++++++++------ 6 files changed, 79 insertions(+), 34 deletions(-) diff --git a/src/cmd/trace/gen.go b/src/cmd/trace/gen.go index 03ee5037e9..6e4d82799e 100644 --- a/src/cmd/trace/gen.go +++ b/src/cmd/trace/gen.go @@ -282,11 +282,11 @@ func (g *globalMetricGenerator) GlobalMetric(ctx *traceContext, ev *trace.Event) m := ev.Metric() switch m.Name { case "/memory/classes/heap/objects:bytes": - ctx.HeapAlloc(ctx.elapsed(ev.Time()), m.Value.Uint64()) + ctx.HeapAlloc(ctx.elapsed(ev.Time()), m.Value.ToUint64()) case "/gc/heap/goal:bytes": - ctx.HeapGoal(ctx.elapsed(ev.Time()), m.Value.Uint64()) + ctx.HeapGoal(ctx.elapsed(ev.Time()), m.Value.ToUint64()) case "/sched/gomaxprocs:threads": - ctx.Gomaxprocs(m.Value.Uint64()) + ctx.Gomaxprocs(m.Value.ToUint64()) } } diff --git a/src/internal/trace/event.go b/src/internal/trace/event.go index ebf8aaa977..896ab7f73a 100644 --- a/src/internal/trace/event.go +++ b/src/internal/trace/event.go @@ -315,12 +315,25 @@ type ExperimentalEvent struct { // Experiment is the name of the experiment this event is a part of. Experiment string - // ArgNames is the names of the event's arguments in order. - // This may refer to a globally shared slice. Copy before mutating. - ArgNames []string + // Args lists the names of the event's arguments in order. + Args []string - // Args contains the event's arguments. - Args []uint64 + // argValues contains the raw integer arguments which are interpreted + // by ArgValue using table. + table *evTable + argValues []uint64 +} + +// ArgValue returns a typed Value for the i'th argument in the experimental event. +func (e ExperimentalEvent) ArgValue(i int) Value { + if i < 0 || i >= len(e.Args) { + panic(fmt.Sprintf("experimental event argument index %d out of bounds [0, %d)", i, len(e.Args))) + } + if strings.HasSuffix(e.Args[i], "string") { + s := e.table.strings.mustGet(stringID(e.argValues[i])) + return stringValue(s) + } + return uint64Value(e.argValues[i]) } // ExperimentalBatch represents a packet of unparsed data along with metadata about that packet. @@ -421,13 +434,13 @@ func (e Event) Metric() Metric { switch e.base.typ { case tracev2.EvProcsChange: m.Name = "/sched/gomaxprocs:threads" - m.Value = Value{kind: ValueUint64, scalar: e.base.args[0]} + m.Value = uint64Value(e.base.args[0]) case tracev2.EvHeapAlloc: m.Name = "/memory/classes/heap/objects:bytes" - m.Value = Value{kind: ValueUint64, scalar: e.base.args[0]} + m.Value = uint64Value(e.base.args[0]) case tracev2.EvHeapGoal: m.Name = "/gc/heap/goal:bytes" - m.Value = Value{kind: ValueUint64, scalar: e.base.args[0]} + m.Value = uint64Value(e.base.args[0]) default: panic(fmt.Sprintf("internal error: unexpected wire-format event type for Metric kind: %d", e.base.typ)) } @@ -503,11 +516,11 @@ func (e Event) RangeAttributes() []RangeAttribute { return []RangeAttribute{ { Name: "bytes swept", - Value: Value{kind: ValueUint64, scalar: e.base.args[0]}, + Value: uint64Value(e.base.args[0]), }, { Name: "bytes reclaimed", - Value: Value{kind: ValueUint64, scalar: e.base.args[1]}, + Value: uint64Value(e.base.args[1]), }, } } @@ -687,8 +700,9 @@ func (e Event) Experimental() ExperimentalEvent { return ExperimentalEvent{ Name: spec.Name, Experiment: tracev2.Experiments()[spec.Experiment], - ArgNames: argNames, - Args: e.base.args[:len(argNames)], + Args: argNames, + table: e.table, + argValues: e.base.args[:len(argNames)], } } @@ -773,7 +787,7 @@ func (e Event) String() string { switch kind := e.Kind(); kind { case EventMetric: m := e.Metric() - fmt.Fprintf(&sb, " Name=%q Value=%s", m.Name, valueAsString(m.Value)) + fmt.Fprintf(&sb, " Name=%q Value=%s", m.Name, m.Value) case EventLabel: l := e.Label() fmt.Fprintf(&sb, " Label=%q Resource=%s", l.Label, l.Resource) @@ -786,7 +800,7 @@ func (e Event) String() string { if i != 0 { fmt.Fprintf(&sb, " ") } - fmt.Fprintf(&sb, "%q=%s", attr.Name, valueAsString(attr.Value)) + fmt.Fprintf(&sb, "%q=%s", attr.Name, attr.Value) } fmt.Fprintf(&sb, "]") } @@ -822,7 +836,14 @@ func (e Event) String() string { } case EventExperimental: r := e.Experimental() - fmt.Fprintf(&sb, " Name=%s ArgNames=%v Args=%v", r.Name, r.ArgNames, r.Args) + fmt.Fprintf(&sb, " Name=%s Args=[", r.Name) + for i, arg := range r.Args { + if i != 0 { + fmt.Fprintf(&sb, ", ") + } + fmt.Fprintf(&sb, "%s=%s", arg, r.ArgValue(i).String()) + } + fmt.Fprintf(&sb, "]") } if stk := e.Stack(); stk != NoStack { fmt.Fprintln(&sb) diff --git a/src/internal/trace/gc.go b/src/internal/trace/gc.go index 46890e784d..f5e8fe79f2 100644 --- a/src/internal/trace/gc.go +++ b/src/internal/trace/gc.go @@ -103,7 +103,7 @@ func MutatorUtilizationV2(events []Event, flags UtilFlags) [][]MutatorUtil { if m.Name != "/sched/gomaxprocs:threads" { break } - gomaxprocs := int(m.Value.Uint64()) + gomaxprocs := int(m.Value.ToUint64()) if len(ps) > gomaxprocs { if flags&UtilPerProc != 0 { // End each P's series. diff --git a/src/internal/trace/testtrace/validation.go b/src/internal/trace/testtrace/validation.go index 3d12f75c49..f61f7a3ffa 100644 --- a/src/internal/trace/testtrace/validation.go +++ b/src/internal/trace/testtrace/validation.go @@ -91,7 +91,7 @@ func (v *Validator) Event(ev trace.Event) error { switch m.Value.Kind() { case trace.ValueUint64: // Just make sure it doesn't panic. - _ = m.Value.Uint64() + _ = m.Value.ToUint64() } case trace.EventLabel: l := ev.Label() diff --git a/src/internal/trace/tracev2/spec.go b/src/internal/trace/tracev2/spec.go index af92865781..6e54c399f4 100644 --- a/src/internal/trace/tracev2/spec.go +++ b/src/internal/trace/tracev2/spec.go @@ -19,7 +19,9 @@ type EventSpec struct { // Its length determines the number of arguments an event has. // // Argument names follow a certain structure and this structure - // is relied on by the testing framework to type-check arguments. + // is relied on by the testing framework to type-check arguments + // and to produce Values for experimental events. + // // The structure is: // // (?P[A-Za-z]+)(_(?P[A-Za-z]+))? diff --git a/src/internal/trace/value.go b/src/internal/trace/value.go index bd2cba7878..bf396b6a9e 100644 --- a/src/internal/trace/value.go +++ b/src/internal/trace/value.go @@ -4,12 +4,16 @@ package trace -import "fmt" +import ( + "fmt" + "unsafe" +) // Value is a dynamically-typed value obtained from a trace. type Value struct { - kind ValueKind - scalar uint64 + kind ValueKind + pointer unsafe.Pointer + scalar uint64 } // ValueKind is the type of a dynamically-typed value from a trace. @@ -18,6 +22,7 @@ type ValueKind uint8 const ( ValueBad ValueKind = iota ValueUint64 + ValueString ) // Kind returns the ValueKind of the value. @@ -30,24 +35,41 @@ func (v Value) Kind() ValueKind { return v.kind } -// Uint64 returns the uint64 value for a MetricSampleUint64. +// ToUint64 returns the uint64 value for a ValueUint64. // -// Panics if this metric sample's Kind is not MetricSampleUint64. -func (v Value) Uint64() uint64 { +// Panics if this Value's Kind is not ValueUint64. +func (v Value) ToUint64() uint64 { if v.kind != ValueUint64 { - panic("Uint64 called on Value of a different Kind") + panic("ToUint64 called on Value of a different Kind") } return v.scalar } -// valueAsString produces a debug string value. +// ToString returns the uint64 value for a ValueString. // -// This isn't just Value.String because we may want to use that to store -// string values in the future. -func valueAsString(v Value) string { +// Panics if this Value's Kind is not ValueString. +func (v Value) ToString() string { + if v.kind != ValueString { + panic("ToString called on Value of a different Kind") + } + return unsafe.String((*byte)(v.pointer), int(v.scalar)) +} + +func uint64Value(x uint64) Value { + return Value{kind: ValueUint64, scalar: x} +} + +func stringValue(s string) Value { + return Value{kind: ValueString, scalar: uint64(len(s)), pointer: unsafe.Pointer(unsafe.StringData(s))} +} + +// String returns the string representation of the value. +func (v Value) String() string { switch v.Kind() { case ValueUint64: - return fmt.Sprintf("Uint64(%d)", v.scalar) + return fmt.Sprintf("Value{Uint64(%d)}", v.ToUint64()) + case ValueString: + return fmt.Sprintf("Value{String(%s)}", v.ToString()) } - return "Bad" + return "Value{Bad}" } -- 2.51.0