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.

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

feat: improve context in error advice messages #467

Merged
merged 1 commit into from
Dec 16, 2024
Merged
Changes from all commits
Commits
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
feat: improve context in error advice messages
Provide more context information in the error messages propagated from
advice application errors, so that they are easier to troubleshoot.
These messages aim to provide a lineage that allows maintainers to know
exactly what advice failed, on what file.
RomainMuller committed Dec 16, 2024

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
commit 295e46fc0f8d7a42bfd256eb634c77826c1bd67c
4 changes: 2 additions & 2 deletions internal/injector/aspect/advice/assign.go
Original file line number Diff line number Diff line change
@@ -26,12 +26,12 @@
func (a *assignValue) Apply(ctx context.AdviceContext) (bool, error) {
spec, ok := ctx.Node().(*dst.ValueSpec)
if !ok {
return false, fmt.Errorf("expected *dst.ValueSpec, got %T", ctx.Node())
return false, fmt.Errorf("assign-value: expected *dst.ValueSpec, got %T", ctx.Node())

Check warning on line 29 in internal/injector/aspect/advice/assign.go

Codecov / codecov/patch

internal/injector/aspect/advice/assign.go#L29

Added line #L29 was not covered by tests
}

expr, err := a.Template.CompileExpression(ctx)
if err != nil {
return false, err
return false, fmt.Errorf("assign-value: %w", err)

Check warning on line 34 in internal/injector/aspect/advice/assign.go

Codecov / codecov/patch

internal/injector/aspect/advice/assign.go#L34

Added line #L34 was not covered by tests
}

spec.Values = make([]dst.Expr, len(spec.Names))
4 changes: 2 additions & 2 deletions internal/injector/aspect/advice/block.go
Original file line number Diff line number Diff line change
@@ -29,12 +29,12 @@
func (a *prependStatements) Apply(ctx context.AdviceContext) (bool, error) {
block, ok := ctx.Node().(*dst.BlockStmt)
if !ok {
return false, fmt.Errorf("expected *dst.BlockStmt, got %T", ctx.Node())
return false, fmt.Errorf("prepend-statements: expected *dst.BlockStmt, got %T", ctx.Node())

Check warning on line 32 in internal/injector/aspect/advice/block.go

Codecov / codecov/patch

internal/injector/aspect/advice/block.go#L32

Added line #L32 was not covered by tests
}

stmts, err := a.Template.CompileBlock(ctx)
if err != nil {
return false, err
return false, fmt.Errorf("prepend-statements: %w", err)

Check warning on line 37 in internal/injector/aspect/advice/block.go

Codecov / codecov/patch

internal/injector/aspect/advice/block.go#L37

Added line #L37 was not covered by tests
}

list := make([]dst.Stmt, 1+len(block.List))
4 changes: 2 additions & 2 deletions internal/injector/aspect/advice/call.go
Original file line number Diff line number Diff line change
@@ -32,15 +32,15 @@
func (a *appendArgs) Apply(ctx context.AdviceContext) (bool, error) {
call, ok := ctx.Node().(*dst.CallExpr)
if !ok {
return false, fmt.Errorf("expected a *dst.CallExpr, received %T", ctx.Node())
return false, fmt.Errorf("append-arguments: expected a *dst.CallExpr, received %T", ctx.Node())

Check warning on line 35 in internal/injector/aspect/advice/call.go

Codecov / codecov/patch

internal/injector/aspect/advice/call.go#L35

Added line #L35 was not covered by tests
}

newArgs := make([]dst.Expr, len(a.Templates))
var err error
for i, t := range a.Templates {
newArgs[i], err = t.CompileExpression(ctx)
if err != nil {
return false, err
return false, fmt.Errorf("append-arguments[%d]: %w", i, err)

Check warning on line 43 in internal/injector/aspect/advice/call.go

Codecov / codecov/patch

internal/injector/aspect/advice/call.go#L43

Added line #L43 was not covered by tests
}
ctx.EnsureMinGoLang(t.Lang)
}
4 changes: 3 additions & 1 deletion internal/injector/aspect/advice/inject.go
Original file line number Diff line number Diff line change
@@ -6,6 +6,8 @@
package advice

import (
"fmt"

"github.com/DataDog/orchestrion/internal/fingerprint"
"github.com/DataDog/orchestrion/internal/injector/aspect/advice/code"
"github.com/DataDog/orchestrion/internal/injector/aspect/context"
@@ -26,7 +28,7 @@
func (a injectDeclarations) Apply(ctx context.AdviceContext) (bool, error) {
decls, err := a.Template.CompileDeclarations(ctx)
if err != nil {
return false, err
return false, fmt.Errorf("inject-declarations: %w", err)

Check warning on line 31 in internal/injector/aspect/advice/inject.go

Codecov / codecov/patch

internal/injector/aspect/advice/inject.go#L31

Added line #L31 was not covered by tests
}

if len(decls) == 0 {
4 changes: 2 additions & 2 deletions internal/injector/aspect/advice/wrap.go
Original file line number Diff line number Diff line change
@@ -33,12 +33,12 @@
ctx = ctx.Child(kve.Value, "Value", -1)
defer ctx.Release()
} else if _, ok = ctx.Node().(dst.Expr); !ok {
return false, fmt.Errorf("expected dst.Expr or *dst.KeyValueExpr, got %T", ctx.Node())
return false, fmt.Errorf("wrap-expression: expected dst.Expr or *dst.KeyValueExpr, got %T", ctx.Node())

Check warning on line 36 in internal/injector/aspect/advice/wrap.go

Codecov / codecov/patch

internal/injector/aspect/advice/wrap.go#L36

Added line #L36 was not covered by tests
}

repl, err := a.Template.CompileExpression(ctx)
if err != nil {
return false, err
return false, fmt.Errorf("wrap-expression: %w", err)

Check warning on line 41 in internal/injector/aspect/advice/wrap.go

Codecov / codecov/patch

internal/injector/aspect/advice/wrap.go#L41

Added line #L41 was not covered by tests
}

if kve == nil {
6 changes: 3 additions & 3 deletions internal/injector/injector.go
Original file line number Diff line number Diff line change
@@ -159,7 +159,7 @@
var err error
result.Modified, result.References, result.GoLang, err = i.applyAspects(decorator, file, i.RootConfig, typeInfo)
if err != nil {
return result, err
return result, fmt.Errorf("%q: %w", result.Filename, err)

Check warning on line 162 in internal/injector/injector.go

Codecov / codecov/patch

internal/injector/injector.go#L162

Added line #L162 was not covered by tests
}

if result.Modified {
@@ -239,14 +239,14 @@
if !inj.JoinPoint.Matches(ctx) {
continue
}
for _, act := range inj.Advice {
for idx, act := range inj.Advice {
var changed bool
changed, err = act.Apply(ctx)
if changed {
mod = true
}
if err != nil {
return
return mod, fmt.Errorf("%q[%d]: %w", inj.ID, idx, err)

Check warning on line 249 in internal/injector/injector.go

Codecov / codecov/patch

internal/injector/injector.go#L249

Added line #L249 was not covered by tests
}
}
}