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

feat: add failure reason to events #4203

Merged
merged 4 commits into from
Nov 14, 2024
Merged

feat: add failure reason to events #4203

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e8ed8cb
feat: add failure reason to events
zepatrik Nov 12, 2024
9b60115
fix: add full error as reason when not matched
zepatrik Nov 13, 2024
2d2daa9
Merge remote-tracking branch 'origin/master' into feat/event-failure-…
zepatrik Nov 13, 2024
62ccfb2
feat: add flow ID to events
zepatrik Nov 13, 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
1 change: 1 addition & 0 deletions internal/client-go/go.sum
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ github.com/golang/protobuf v1.2.0/go.mod h1:6lQm79b+lXiMfvg/cZm0SGofjICqVBUtrP5y
golang.org/x/net v0.0.0-20180724234803-3673e40ba225/go.mod h1:mL1N/T3taQHkDXs73rZJwtUhF3w3ftmwwsq0BUmARs4=
golang.org/x/net v0.0.0-20190108225652-1e06a53dbb7e h1:bRhVy7zSSasaqNksaRZiA5EEI+Ei4I1nO5Jh72wfHlg=
golang.org/x/net v0.0.0-20190108225652-1e06a53dbb7e/go.mod h1:mL1N/T3taQHkDXs73rZJwtUhF3w3ftmwwsq0BUmARs4=
golang.org/x/oauth2 v0.0.0-20190604053449-0f29369cfe45/go.mod h1:gOpvHmFTYa4IltrdGE7lF6nIHvwfUNPOp7c8zoXwtLw=
golang.org/x/sync v0.0.0-20181221193216-37e7f081c4d4 h1:YUO/7uOKsKeq9UokNS62b8FYywz3ker1l1vDZRCRefw=
golang.org/x/sync v0.0.0-20181221193216-37e7f081c4d4/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM=
golang.org/x/text v0.3.0/go.mod h1:NqM8EUOU14njkJ3fqMW+pc6Ldnwhi/IjpwHt7yyuwOQ=
Expand Down
4 changes: 2 additions & 2 deletions selfservice/flow/login/error.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,12 +88,12 @@ func (s *ErrorHandler) WriteFlowError(w http.ResponseWriter, r *http.Request, f
Info("Encountered self-service login error.")

if f == nil {
trace.SpanFromContext(r.Context()).AddEvent(events.NewLoginFailed(r.Context(), "", "", false))
trace.SpanFromContext(r.Context()).AddEvent(events.NewLoginFailed(r.Context(), "", "", false, err))
s.forward(w, r, nil, err)
return
}

trace.SpanFromContext(r.Context()).AddEvent(events.NewLoginFailed(r.Context(), string(f.Type), string(f.RequestedAAL), f.Refresh))
trace.SpanFromContext(r.Context()).AddEvent(events.NewLoginFailed(r.Context(), string(f.Type), string(f.RequestedAAL), f.Refresh, err))

if expired, inner := s.PrepareReplacementForExpiredFlow(w, r, f, err); inner != nil {
s.WriteFlowError(w, r, f, group, inner)
Expand Down
4 changes: 2 additions & 2 deletions selfservice/flow/recovery/error.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,12 +73,12 @@ func (s *ErrorHandler) WriteFlowError(
Info("Encountered self-service recovery error.")

if f == nil {
trace.SpanFromContext(r.Context()).AddEvent(events.NewRecoveryFailed(r.Context(), "", ""))
trace.SpanFromContext(r.Context()).AddEvent(events.NewRecoveryFailed(r.Context(), "", "", recoveryErr))
s.forward(w, r, nil, recoveryErr)
return
}

trace.SpanFromContext(r.Context()).AddEvent(events.NewRecoveryFailed(r.Context(), string(f.Type), f.Active.String()))
trace.SpanFromContext(r.Context()).AddEvent(events.NewRecoveryFailed(r.Context(), string(f.Type), f.Active.String(), recoveryErr))

if expiredError := new(flow.ExpiredError); errors.As(recoveryErr, &expiredError) {
strategy, err := s.d.RecoveryStrategies(r.Context()).Strategy(f.Active.String())
Expand Down
4 changes: 2 additions & 2 deletions selfservice/flow/registration/error.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,11 +93,11 @@ func (s *ErrorHandler) WriteFlowError(
Info("Encountered self-service flow error.")

if f == nil {
trace.SpanFromContext(r.Context()).AddEvent(events.NewRegistrationFailed(r.Context(), "", ""))
trace.SpanFromContext(r.Context()).AddEvent(events.NewRegistrationFailed(r.Context(), "", "", err))
s.forward(w, r, nil, err)
return
}
trace.SpanFromContext(r.Context()).AddEvent(events.NewRegistrationFailed(r.Context(), string(f.Type), f.Active.String()))
trace.SpanFromContext(r.Context()).AddEvent(events.NewRegistrationFailed(r.Context(), string(f.Type), f.Active.String(), err))

if expired, inner := s.PrepareReplacementForExpiredFlow(w, r, f, err); inner != nil {
s.forward(w, r, f, err)
Expand Down
4 changes: 2 additions & 2 deletions selfservice/flow/settings/error.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -180,11 +180,11 @@ func (s *ErrorHandler) WriteFlowError(
}

if f == nil {
trace.SpanFromContext(ctx).AddEvent(events.NewSettingsFailed(ctx, "", ""))
trace.SpanFromContext(ctx).AddEvent(events.NewSettingsFailed(ctx, "", "", err))
s.forward(ctx, w, r, nil, err)
return
}
trace.SpanFromContext(ctx).AddEvent(events.NewSettingsFailed(ctx, string(f.Type), f.Active.String()))
trace.SpanFromContext(ctx).AddEvent(events.NewSettingsFailed(ctx, string(f.Type), f.Active.String(), err))

if expired, inner := s.PrepareReplacementForExpiredFlow(ctx, w, r, f, id, err); inner != nil {
s.forward(ctx, w, r, f, err)
Expand Down
4 changes: 2 additions & 2 deletions selfservice/flow/verification/error.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,11 +69,11 @@ func (s *ErrorHandler) WriteFlowError(
Info("Encountered self-service verification error.")

if f == nil {
trace.SpanFromContext(r.Context()).AddEvent(events.NewVerificationFailed(r.Context(), "", ""))
trace.SpanFromContext(r.Context()).AddEvent(events.NewVerificationFailed(r.Context(), "", "", err))
s.forward(w, r, nil, err)
return
}
trace.SpanFromContext(r.Context()).AddEvent(events.NewVerificationFailed(r.Context(), string(f.Type), f.Active.String()))
trace.SpanFromContext(r.Context()).AddEvent(events.NewVerificationFailed(r.Context(), string(f.Type), f.Active.String(), err))

if e := new(flow.ExpiredError); errors.As(err, &e) {
strategy, err := s.d.VerificationStrategies(r.Context()).Strategy(f.Active.String())
Expand Down
43 changes: 34 additions & 9 deletions x/events/events.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,14 @@ package events

import (
"context"
"errors"
"fmt"
"net/url"
"time"

"github.com/ory/herodot"
"github.com/ory/kratos/schema"

"github.com/gofrs/uuid"
otelattr "go.opentelemetry.io/otel/attribute"
"go.opentelemetry.io/otel/trace"
Expand Down Expand Up @@ -56,6 +61,7 @@ const (
attributeKeyWebhookResponseStatusCode semconv.AttributeKey = "WebhookResponseStatusCode"
attributeKeyWebhookAttemptNumber semconv.AttributeKey = "WebhookAttemptNumber"
attributeKeyWebhookRequestID semconv.AttributeKey = "WebhookRequestID"
attributeKeyReason semconv.AttributeKey = "Reason"
)

func attrSessionID(val uuid.UUID) otelattr.KeyValue {
Expand Down Expand Up @@ -118,6 +124,10 @@ func attrWebhookRequestID(id uuid.UUID) otelattr.KeyValue {
return otelattr.String(attributeKeyWebhookRequestID.String(), id.String())
}

func attrReason(err error) otelattr.KeyValue {
return otelattr.String(attributeKeyReason.String(), reasonForError(err))
}

func NewSessionIssued(ctx context.Context, aal string, sessionID, identityID uuid.UUID) (string, trace.EventOption) {
return SessionIssued.String(),
trace.WithAttributes(
Expand Down Expand Up @@ -176,7 +186,7 @@ func NewLoginSucceeded(ctx context.Context, o *LoginSucceededOpts) (string, trac
)
}

func NewRegistrationSucceeded(ctx context.Context, identityID uuid.UUID, flowType string, method, provider string) (string, trace.EventOption) {
func NewRegistrationSucceeded(ctx context.Context, identityID uuid.UUID, flowType, method, provider string) (string, trace.EventOption) {
return RegistrationSucceeded.String(),
trace.WithAttributes(append(
semconv.AttributesFromContext(ctx),
Expand All @@ -187,7 +197,7 @@ func NewRegistrationSucceeded(ctx context.Context, identityID uuid.UUID, flowTyp
)...)
}

func NewRecoverySucceeded(ctx context.Context, identityID uuid.UUID, flowType string, method string) (string, trace.EventOption) {
func NewRecoverySucceeded(ctx context.Context, identityID uuid.UUID, flowType, method string) (string, trace.EventOption) {
return RecoverySucceeded.String(),
trace.WithAttributes(append(
semconv.AttributesFromContext(ctx),
Expand All @@ -197,7 +207,7 @@ func NewRecoverySucceeded(ctx context.Context, identityID uuid.UUID, flowType st
)...)
}

func NewSettingsSucceeded(ctx context.Context, identityID uuid.UUID, flowType string, method string) (string, trace.EventOption) {
func NewSettingsSucceeded(ctx context.Context, identityID uuid.UUID, flowType, method string) (string, trace.EventOption) {
return SettingsSucceeded.String(),
trace.WithAttributes(append(
semconv.AttributesFromContext(ctx),
Expand All @@ -207,7 +217,7 @@ func NewSettingsSucceeded(ctx context.Context, identityID uuid.UUID, flowType st
)...)
}

func NewVerificationSucceeded(ctx context.Context, identityID uuid.UUID, flowType string, method string) (string, trace.EventOption) {
func NewVerificationSucceeded(ctx context.Context, identityID uuid.UUID, flowType, method string) (string, trace.EventOption) {
return VerificationSucceeded.String(),
trace.WithAttributes(append(
semconv.AttributesFromContext(ctx),
Expand All @@ -217,39 +227,43 @@ func NewVerificationSucceeded(ctx context.Context, identityID uuid.UUID, flowTyp
)...)
}

func NewRegistrationFailed(ctx context.Context, flowType string, method string) (string, trace.EventOption) {
func NewRegistrationFailed(ctx context.Context, flowType, method string, err error) (string, trace.EventOption) {
return RegistrationFailed.String(),
trace.WithAttributes(append(
semconv.AttributesFromContext(ctx),
attrSelfServiceFlowType(flowType),
attrSelfServiceMethodUsed(method),
attrReason(err),
)...)
}

func NewRecoveryFailed(ctx context.Context, flowType string, method string) (string, trace.EventOption) {
func NewRecoveryFailed(ctx context.Context, flowType, method string, err error) (string, trace.EventOption) {
return RecoveryFailed.String(),
trace.WithAttributes(append(
semconv.AttributesFromContext(ctx),
attrSelfServiceFlowType(flowType),
attrSelfServiceMethodUsed(method),
attrReason(err),
)...)
}

func NewSettingsFailed(ctx context.Context, flowType string, method string) (string, trace.EventOption) {
func NewSettingsFailed(ctx context.Context, flowType, method string, err error) (string, trace.EventOption) {
return SettingsFailed.String(),
trace.WithAttributes(append(
semconv.AttributesFromContext(ctx),
attrSelfServiceFlowType(flowType),
attrSelfServiceMethodUsed(method),
attrReason(err),
)...)
}

func NewVerificationFailed(ctx context.Context, flowType string, method string) (string, trace.EventOption) {
func NewVerificationFailed(ctx context.Context, flowType, method string, err error) (string, trace.EventOption) {
return VerificationFailed.String(),
trace.WithAttributes(append(
semconv.AttributesFromContext(ctx),
attrSelfServiceFlowType(flowType),
attrSelfServiceMethodUsed(method),
attrReason(err),
)...)
}

Expand Down Expand Up @@ -283,13 +297,14 @@ func NewIdentityUpdated(ctx context.Context, identityID uuid.UUID) (string, trac
)
}

func NewLoginFailed(ctx context.Context, flowType string, requestedAAL string, isRefresh bool) (string, trace.EventOption) {
func NewLoginFailed(ctx context.Context, flowType, requestedAAL string, isRefresh bool, err error) (string, trace.EventOption) {
return LoginFailed.String(),
trace.WithAttributes(append(
semconv.AttributesFromContext(ctx),
attrSelfServiceFlowType(flowType),
attLoginRequestedAAL(requestedAAL),
attLoginRequestedPrivilegedSession(isRefresh),
attrReason(err),
)...)
}

Expand Down Expand Up @@ -356,3 +371,13 @@ func NewWebhookFailed(ctx context.Context, err error) (string, trace.EventOption
)...,
)
}

func reasonForError(err error) string {
if ve := new(schema.ValidationError); errors.As(err, &ve) {
return ve.Message
}
if r := *new(herodot.ReasonCarrier); errors.As(err, &r) {
return r.Reason()
}
return fmt.Sprintf("reason could not be determined for error: %T", err)
}
Copy link
Member

Choose a reason for hiding this comment

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

People will ask about "reason IDs" to find e.g. all failed logins for reason X over time. Don't our validation errors have some type of ID or error code that we could emit?

Copy link
Member

Choose a reason for hiding this comment

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

I want to avoid people relying on these errors and then we change the casing or error message and we get complaints about backwards incompatible changes.

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree here. We might be able to reuse the text package message IDs, although we might need to add a few more there as not all errors have a message associated.
Should we then split it into two attributes, ReasonID and Reason?

Copy link
Member

Choose a reason for hiding this comment

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

Yes please

Copy link
Member

Choose a reason for hiding this comment

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

We can also set some default like "contact_support" or something for the reason id to make it clear that you should request a reason id if it doesn't exist (opposed to reading the reason string field)

Loading