signalfx / signalfx-go

Go client library and instrumentation bindings for SignalFx
https://www.signalfx.com
Apache License 2.0
14 stars 48 forks source link

adding request context #88

Closed yiwen-stripe closed 4 years ago

yiwen-stripe commented 4 years ago

We'd like to add tracing to some of our requests coming from this library. Unfortunately, it seems that none of the endpoints we are using utilize context (detector api), which means we can't pass that information through. Can we add some contexts to the API (or perhaps add new functions that utilize context)?

cory-signalfx commented 4 years ago

Yeah, that seems reasonable! I'm not sure how quickly I can get to it, but keep an eye out for a PR!

choo-stripe commented 4 years ago

thanks cory!

cory-signalfx commented 4 years ago

Hey @yiwen-stripe and @choo-stripe! Could you volunteer some places where this would be helpful to you in the near term? Obviously I could add it as a parameter, but I imagine you want either something to happen to that context? :)

choo-stripe commented 4 years ago

Howdy! Recently I used https://godoc.org/github.com/signalfx/signalfx-go#Client.GetDetectorIncidents and was scratching my head as to where the WithContext variant was. I didn't PR it because I assumed I would've had to add for all functions, which I didn't have time for. I wonder if we could codemod this.

Not sure if Yiwen has more to add

cory-signalfx commented 4 years ago

But merely adding the context doesn't really do anything, right? Unless I look into it for cancelation or do some tracing stuff, this is a null op?

choo-stripe commented 4 years ago

Right - that was it - I was hoping to add context to the request here: https://github.com/signalfx/signalfx-go/blob/ceee8d2570d5842692c54cb920ed88a76db2586b/client.go#L93

Basically using https://golang.org/pkg/net/http/#NewRequestWithContext

We have generic tracing for all reqs, and enforce that egress is traced. We do this by adding context to the requests and shimming the Dial function in the http.Client.

I was able to hack around this, but adding contexts to requests seems like a fairly standard feature these days.

cory-signalfx commented 4 years ago

Ah, ok. That makes sense. Thanks for clarifying.

cory-signalfx commented 4 years ago

Done!

choo-stripe commented 4 years ago

You're an absolute wizard. Pls come back soon.

cory-signalfx commented 4 years ago

It was your idea, not mine!

lol :)