Support custom context.Context for the APIs

[gunturaf]

I think it's good to have a new wrapper for each APIs that supports a custom context.Context so that we (API users) can inject our own telemetry/traces or implement a timeout/deadline/early cancellation for each calls.

For example, for this function

go-crisp-api/crisp/website_people.go

Lines 312 to 323 in 0d8801d

	func (service *WebsiteService) ListSuggestedPeopleSegments(websiteID string, pageNumber uint) (*[]PeopleSuggestedSegment, *Response, error) {
	url := fmt.Sprintf("website/%s/people/suggest/segments/%d", websiteID, pageNumber)
	req, _ := service.client.NewRequest("GET", url, nil)
	segments := new(PeopleSuggestedSegmentData)
	resp, err := service.client.Do(req, segments)
	if err != nil {
	return nil, resp, err
	}
	return segments.Data, resp, err
	}

we can change it into:

// ListSuggestedPeopleSegmentsContext lists suggested segments for people.
func (service *WebsiteService) ListSuggestedPeopleSegmentsContext(ctx context.Context, websiteID string, pageNumber uint) (*[]PeopleSuggestedSegment, *Response, error) {
  url := fmt.Sprintf("website/%s/people/suggest/segments/%d", websiteID, pageNumber)
  req, _ := service.client.NewRequestContext(ctx, "GET", url, nil)

  segments := new(PeopleSuggestedSegmentData)
  resp, err := service.client.Do(req, segments)
  if err != nil {
    return nil, resp, err
  }

  return segments.Data, resp, err
}

// ListSuggestedPeopleSegments lists suggested segments for people.
func (service *WebsiteService) ListSuggestedPeopleSegments(websiteID string, pageNumber uint) (*[]PeopleSuggestedSegment, *Response, error) {
  return ListSuggestedPeopleSegmentsContext(context.Background(), websiteID, pageNumber)
}

With this approach, existing users can use the current APIs, it won't break them since we create new APIs instead of adding another parameter.

This approach also used in other libraries such as this one https://github.com/jmoiron/sqlx/blob/874a5d4c14788a7ade895f913e4ae947a6354922/sqlx_context.go#L54-L62

I can make the PR for this if you folks in the same page on this. cc: @valeriansaliou

[baptistejamin]

Hello there! Thank you for the feedback. I will forward this to the tech team and we will get back to you for updates if possible.

[valeriansaliou]

Hello, thanks for that suggestion.

While I understand the purpose of this proposal, given the amount of API wrapper functions we have, this would basically increase the library code size by ~30%, and add extra maintenance effort and work when adding new routes, on my side

We don’t need this for our own use at Crisp, and this library is mostly currently used for our own internal plugins.

So, given the added hurdles for the benefit for us, I am not sure I’d agree onto adding this to the library.

What do you need to debug specifically?

[gunturaf]

We implemented distributed tracing, this causes every external operations such as DB access and third party HTTP APIs to be traced, so when a specific operation fails or have noticeable latency we can properly pinpoint the root cause.

Distributed tracing providers such as NewRelic (or OpenTelemetry) creates new Transaction (the "trace") for every incoming HTTP/gRPC request and injected the Transaction into the incoming request ctx. With that fact, we can simply reuse the ctx to enable auto-instrumentation for most operations like MySQL, Elasticsearch, or an outgoing HTTP request.

We created a custom http.RoundTripper (based on this) that we passed into Crisp's func NewWithConfig(config ClientConfig) *Client. Our RoundTripper will read the injected Transaction in the ctx to wrap the actual "http hit" with new Span so it properly traced. This pattern of reusing ctx is also used on many libraries that supports distributed tracing.

For our use case, we need to debug/enable visibility for our Crisp calls, so that if our server cannot reach Crisp (by any reason) or when we need to improve our Crisp calls (such as offloading into goroutines), then our tech team can automatically alerted. Because this library doesn't currently support custom context.Context, we cannot leverage the auto-instrumentation via http.RoundTripper, so that (for now) we wrap our use cases manually to enable traces for Crisp calls.