aws / aws-xray-sdk-go

AWS X-Ray SDK for the Go programming language.
Apache License 2.0
276 stars 117 forks source link

Feature request: Don't panic when a subsegment can't be found #334

Open a-h opened 2 years ago

a-h commented 2 years ago

When I run this code on my local machine, I don't expect a panic.

package main

import (
    "fmt"
    "io"
    "os"

    "github.com/aws/aws-xray-sdk-go/xray"
)

func main() {
    c := xray.Client(nil)
    resp, err := c.Get("https://google.com")
    if err != nil {
        fmt.Printf("failed to access Google: %v\n", err)
        os.Exit(1)
    }
    io.Copy(os.Stdout, resp.Body)
}

But it panics!

panic: failed to begin subsegment named 'google.com': segment cannot be found.

I was hunting around in the docs to find out how to suppress this behaviour. I'd like my code to use X-Ray, if it's available, but if it's not, I don't care.

I found that I can set an environment variable, but that's a pain, because I'd even have to set that environment variable to run some unit tests or integration tests locally.

AWS_XRAY_CONTEXT_MISSING=IGNORE_ERROR

As per the Go Proverbs, don't panic please. 😁

Suggestions:

bhautikpip commented 2 years ago

Hey @a-h,

Looks like panic is expected in your code because you're trying to create a subsegment without creating a segment first. Ideally either you should create manual segment using xray.BeginSegment API or maybe wrap your HTTP client around HTTP handler instrumentation (examples can be found in README) then X-Ray Go SDK will automatically creates a segment and then Go SDK will create HTTP subsegment. Yeah you're right you can avoid panic using IGNORE_ERROR option which would just ignore the panic and continue execution of your code.

a-h commented 2 years ago

Thanks @bhautikpip, I understand why it happens, that's not the point of the issue.

I'm saying that it's a footgun that the default behaviour of the library is to panic over nothing.

The current default behaviour is not expected, see https://go.dev/blog/defer-panic-and-recover and https://go.dev/blog/errors-are-values

The convention in the Go libraries is that even when a package uses panic internally, its external API still presents explicit error return values.

i.e. the API should return an error value (or do nothing) rather than panic. It's total overkill to crash the program just because an XRay subsegment can't be created.

bhautikpip commented 2 years ago

Yeah that's fair. Right now xray.BeginSubSegment API would panic if it does not find it's parent segment. To avoid this with the current state of the SDK you can set AWS_XRAY_CONTEXT_MISSING as IGNORE_ERROR or LOG_ERROR. I can mark this ask as an enhancement.

joerdav commented 1 year ago

I'd love to see this SDK follow an error handling pattern that is inline with what the Go community agrees on.