From e9c5ea242370640778507b8a542dd40de4fa1d01 Mon Sep 17 00:00:00 2001 From: Robert Findley Date: Tue, 16 Apr 2024 12:56:26 -0400 Subject: [PATCH] gopls: normalize logging attributes As part of the log audit in golang/go#66746, I noticed several irregularities addressed by this CL: - Move "tags" (domain-specific event keys, which are used construct labels) closer to the packages that use them. Specifically, the internal/event/tag package contained gopls-specific tags, but x/tools should not care about gopls. - Use consistent values for log attributes. For example, "method" was being used to mean jsonrpc2 method and Go method. Also, "directory" was being used as both file path and URI. - Use log attributes for the view attributes logged when views are created. - Eliminate (yet another) redundant log during Load. - Include the ViewID with snapshot.Labels, since snapshot IDs are only meaningful relative to a View. With these changes, my audit of logging is complete. Fixes golang/go#66746 Change-Id: Iaa60797a7412fb8e222e78e2e58eff2da9563bbb Reviewed-on: https://go-review.googlesource.com/c/tools/+/579335 LUCI-TryBot-Result: Go LUCI Reviewed-by: Alan Donovan --- internal/jsonrpc2/conn.go | 32 ++++++++++++++++---------------- 1 file changed, 16 insertions(+), 16 deletions(-) diff --git a/internal/jsonrpc2/conn.go b/internal/jsonrpc2/conn.go index df91550..45bbd8b 100644 --- a/internal/jsonrpc2/conn.go +++ b/internal/jsonrpc2/conn.go @@ -17,7 +17,7 @@ import ( "golang.org/x/tools/internal/event" "golang.org/x/tools/internal/event/keys" "golang.org/x/tools/internal/event/label" - "golang.org/x/tools/internal/event/tag" + "golang.org/x/tools/internal/jsonrpc2" ) // Binder builds a connection configuration. @@ -262,8 +262,8 @@ func newConnection(bindCtx context.Context, rwc io.ReadWriteCloser, binder Binde // be handed to the method invoked. func (c *Connection) Notify(ctx context.Context, method string, params interface{}) (err error) { ctx, done := event.Start(ctx, method, - tag.Method.Of(method), - tag.RPCDirection.Of(tag.Outbound), + jsonrpc2.Method.Of(method), + jsonrpc2.RPCDirection.Of(jsonrpc2.Outbound), ) attempted := false @@ -300,7 +300,7 @@ func (c *Connection) Notify(ctx context.Context, method string, params interface return fmt.Errorf("marshaling notify parameters: %v", err) } - event.Metric(ctx, tag.Started.Of(1)) + event.Metric(ctx, jsonrpc2.Started.Of(1)) return c.write(ctx, notify) } @@ -313,9 +313,9 @@ func (c *Connection) Call(ctx context.Context, method string, params interface{} // Generate a new request identifier. id := Int64ID(atomic.AddInt64(&c.seq, 1)) ctx, endSpan := event.Start(ctx, method, - tag.Method.Of(method), - tag.RPCDirection.Of(tag.Outbound), - tag.RPCID.Of(fmt.Sprintf("%q", id)), + jsonrpc2.Method.Of(method), + jsonrpc2.RPCDirection.Of(jsonrpc2.Outbound), + jsonrpc2.RPCID.Of(fmt.Sprintf("%q", id)), ) ac := &AsyncCall{ @@ -349,7 +349,7 @@ func (c *Connection) Call(ctx context.Context, method string, params interface{} return ac } - event.Metric(ctx, tag.Started.Of(1)) + event.Metric(ctx, jsonrpc2.Started.Of(1)) if err := c.write(ctx, call); err != nil { // Sending failed. We will never get a response, so deliver a fake one if it // wasn't already retired by the connection breaking. @@ -539,16 +539,16 @@ func (c *Connection) readIncoming(ctx context.Context, reader Reader, preempter func (c *Connection) acceptRequest(ctx context.Context, msg *Request, msgBytes int64, preempter Preempter) { // Add a span to the context for this request. labels := append(make([]label.Label, 0, 3), // Make space for the ID if present. - tag.Method.Of(msg.Method), - tag.RPCDirection.Of(tag.Inbound), + jsonrpc2.Method.Of(msg.Method), + jsonrpc2.RPCDirection.Of(jsonrpc2.Inbound), ) if msg.IsCall() { - labels = append(labels, tag.RPCID.Of(fmt.Sprintf("%q", msg.ID))) + labels = append(labels, jsonrpc2.RPCID.Of(fmt.Sprintf("%q", msg.ID))) } ctx, endSpan := event.Start(ctx, msg.Method, labels...) event.Metric(ctx, - tag.Started.Of(1), - tag.ReceivedBytes.Of(msgBytes)) + jsonrpc2.Started.Of(1), + jsonrpc2.ReceivedBytes.Of(msgBytes)) // In theory notifications cannot be cancelled, but we build them a cancel // context anyway. @@ -754,7 +754,7 @@ func (c *Connection) write(ctx context.Context, msg Message) error { writer := <-c.writer defer func() { c.writer <- writer }() n, err := writer.Write(ctx, msg) - event.Metric(ctx, tag.SentBytes.Of(n)) + event.Metric(ctx, jsonrpc2.SentBytes.Of(n)) if err != nil && ctx.Err() == nil { // The call to Write failed, and since ctx.Err() is nil we can't attribute @@ -794,9 +794,9 @@ func (c *Connection) internalErrorf(format string, args ...interface{}) error { // labelStatus labels the status of the event in ctx based on whether err is nil. func labelStatus(ctx context.Context, err error) { if err == nil { - event.Label(ctx, tag.StatusCode.Of("OK")) + event.Label(ctx, jsonrpc2.StatusCode.Of("OK")) } else { - event.Label(ctx, tag.StatusCode.Of("ERROR")) + event.Label(ctx, jsonrpc2.StatusCode.Of("ERROR")) } }