search

graph-gophers / dataloader

Implementation of Facebook's DataLoader in Golang
MIT License
1.21k stars 75 forks source link

add opencensus tracer #57

Closed renatoaquino closed 5 years ago

renatoaquino commented 5 years ago

This PR aims to add a Tracer that uses the OpenCensus standard.

codecov-io commented 5 years ago

Codecov Report

Merging #57 into master will decrease coverage by 6.36%. The diff coverage is 0%.

@@            Coverage Diff             @@
##           master      #57      +/-   ##
==========================================
- Coverage   86.94%   80.57%   -6.37%     
==========================================
  Files           6        6              
  Lines         291      314      +23     
==========================================
  Hits          253      253              
- Misses         38       61      +23
nicksrandall commented 5 years ago

Thanks for your contribution.

I feel like this change might not be needed because it can be implemented in user-space (that's why we have the Tracer interface). As such, I don't want to maintain all the possible tracers that people want to use with this library. Perhaps it would be better to contribute an example of how to integrate this library with OpenCensus rather than baking it in?

If you feel strongly that we should be supporting OpenCensus OVER OpenTracing then I'm open to hearing the argument. I'm not familiar with OpenCensus. Does it have distinct advantages over OpenTracing?

renatoaquino commented 5 years ago

Hello, Nick.

I really think you're right about not adding another dependency. I'll write a markdown file with instructions on how to implement the Trace interface to another tracing backend. Thinking about what you said, I really don't think you should support any specific backend at all; It would only add dependencies to you library and make things harder with go mod into the future.

About OpenCensus and OpenTracing, they are so similar that they are merging into what will be called OpenTelemetry.

I'll close this PR and write a new one with instructions.

Thank's for sharing your work with us.

nicksrandall commented 5 years ago

I agree that it would be ideal to not support any tracer and to leave that as an exercise to the user. Perhaps next time we have a breaking change, I’ll remove OpenTracing from the module.