Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fix span tagging for application error in GO YARPC/gRPC #2287

Open
wants to merge 9 commits into
base: dev
Choose a base branch
from
Open

Fix span tagging for application error in GO YARPC/gRPC #2287

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
d9fe325
saving
kl943 Aug 26, 2024
c3e0cbe
saving
kl943 Aug 26, 2024
8d2c6cf
saving
kl943 Aug 26, 2024
a41057e
saving
kl943 Aug 26, 2024
502479e
saving
kl943 Aug 26, 2024
67ff9f6
saving
kl943 Aug 26, 2024
464cf64
saving
kl943 Sep 5, 2024
6ccb1d0
saving
kl943 Sep 5, 2024
bdfb7f0
saving
kl943 Sep 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 20 additions & 0 deletions api/transport/propagation.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ package transport

import (
"context"
"go.uber.org/yarpc/yarpcerrors"
kexiongliu123 marked this conversation as resolved.
Show resolved Hide resolved
"time"

"github.com/opentracing/opentracing-go"
Expand All @@ -37,6 +38,11 @@ type CreateOpenTracingSpan struct {
ExtraTags opentracing.Tags
}

const (
//TracingTagStatusCode is the span tag key for the YAPRC status code.
kexiongliu123 marked this conversation as resolved.
Show resolved Hide resolved
TracingTagStatusCode = "rpc.yarpc.status_code"
)

// Do creates a new context that has a reference to the started span.
// This should be called before a Outbound makes a call
func (c *CreateOpenTracingSpan) Do(
Expand Down Expand Up @@ -119,3 +125,17 @@ func UpdateSpanWithErr(span opentracing.Span, err error) error {
}
return err
}

// UpdateSpanWithoutErrMsg sets the error tag and status code on a span.
// The error message is intentionally omitted to prevent exposing
// personally identifiable information (PII) in tracing systems.
// Returns the given error
kexiongliu123 marked this conversation as resolved.
Show resolved Hide resolved
func UpdateSpanWithoutErrMsg(span opentracing.Span, err error, errCode yarpcerrors.Code) {
if err != nil {
span.SetTag("error", true)
kexiongliu123 marked this conversation as resolved.
Show resolved Hide resolved
span.SetTag(TracingTagStatusCode, errCode)
span.LogFields(
opentracinglog.String("event", "error"),
)
}
}
14 changes: 14 additions & 0 deletions internal/observability/call.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ package observability
import (
"context"
"fmt"
"github.com/opentracing/opentracing-go"
Copy link
Collaborator

Choose a reason for hiding this comment

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

All such imports should be sorted, thx.

"time"

"go.uber.org/yarpc/api/transport"
Expand Down Expand Up @@ -135,6 +136,8 @@ func (c call) endWithAppError(
res callResult,
extraLogFields ...zap.Field) {
elapsed := _timeNow().Sub(c.started)
// Emit application error to span tag if applicable
c.emitSpanErrorTag(res)
c.endLogs(elapsed, res.err, res.isApplicationError, res.applicationErrorMeta, extraLogFields...)
c.endStats(elapsed, res)
}
Expand Down Expand Up @@ -496,6 +499,17 @@ func (c call) logStreamEvent(err error, success bool, succMsg, errMsg string, ex
ce.Write(fields...)
}

// emitSpanErrorTag sets the error information as tags on the current span
func (c call) emitSpanErrorTag(res callResult) {
kexiongliu123 marked this conversation as resolved.
Show resolved Hide resolved
if span := opentracing.SpanFromContext(c.ctx); span != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

My concern of using obs middleware is that the required inbound/ outbound spans are not available in go context when you are trying to obtain spans here.

Could you double check?

Copy link
Author

Choose a reason for hiding this comment

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

After reviewing the code more thoroughly, I believe you're right. It doesn’t seem straightforward to pass the original spans here. Do you have any suggestions on how we might approach this?

// emit application error to span tag if applicable
if res.isApplicationError {
kexiongliu123 marked this conversation as resolved.
Show resolved Hide resolved
transport.UpdateSpanWithoutErrMsg(span,
res.err, yarpcerrors.FromError(res.err).Code())
kexiongliu123 marked this conversation as resolved.
Show resolved Hide resolved
}
}
}

// inteded for metric tags, this returns the yarpcerrors.Status error code name
// or "unknown_internal_yarpc"
func errToMetricString(err error) string {
Expand Down