Add go-kit instrumentation library

[sagikazarmark]

Fixes #434

Tasks

• code complete with tests passing
• Dockerfile with example app showing instrumentation (alternatively link to https://github.com/sagikazarmark/modern-go-application which serves as an example app for Go kit, showcasing OpenTelemetry as well)
• documentation updated
• instrumentation added to the opentelemetry registry

[sagikazarmark]

This PR is not ready yet: more tests and some examples are missing, but I wanted to submit the PR early so I can get some feedback.

I also wanted to ask: event recording does not seem to be implemented in the mock span. Is it on purpose? Thanks! Solved in 0.14.0

[sagikazarmark]

Thanks for the review @matej-g

I think it's enough for me as a sanity check to finish the PR, so an approver can also review and merge it.

[sagikazarmark]

I was finally able to finish this PR: improved test coverage, added an example based on gorilla and updated the documentation.

I think it's ready to be merged.

What is the procedure for adding it to the registry?

[matej-g]

Nice work @sagikazarmark, this PR looks complete to me now.

What is the procedure for adding it to the registry?

There is another repository for opentelemetry.io, there you can find the "registry" under https://github.com/open-telemetry/opentelemetry.io/tree/master/content/en/registry. Basically once this PR is merged, you can create a PR for that repository which will add a new entry, something like instrumentation-go-kit.md, to represent your instrumentation package. You can checkout the other registry entries as a reference to know what your .md file should include.

[sagikazarmark]

Thanks @matej-g

I guess adding it to the registry is not a task for this PR, so I crossed it off.

[sagikazarmark]

I've rebased the PR to use the latest version. Is there anything I can do to get this merged? I'd really like to avoid manually rebasing and updating the version. :)

[sagikazarmark]

Happy new year everyone!

Friendly ping to see if we can get this merged before the next release. 🙂

[sagikazarmark]

Hey folks,

Can someone merge this PR please? I'd really like to stop rebasing it, it's a waste of effort. It's been approved by three reviewers. What else do I have to do to get it merged?

Thanks!

[codecov]

Codecov Report

Merging #456 (e23a072) into main (f82555b) will increase coverage by 0.1%.
The diff coverage is 85.0%.

@@           Coverage Diff           @@
##            main    #456     +/-   ##
=======================================
+ Coverage   77.9%   78.1%   +0.1%     
=======================================
  Files         55      57      +2     
  Lines       2602    2664     +62     
=======================================
+ Hits        2029    2082     +53     
- Misses       443     451      +8     
- Partials     130     131      +1     

Impacted Files	Coverage Δ
detectors/gcp/gce.go	7.3% <0.0%> (ø)
detectors/gcp/gke.go	0.0% <0.0%> (ø)
...ion/github.com/Shopify/sarama/otelsarama/option.go	100.0% <ø> (ø)
...ation/github.com/astaxie/beego/otelbeego/common.go	100.0% <ø> (ø)
...ation/google.golang.org/grpc/otelgrpc/grpctrace.go	58.3% <40.0%> (-8.4%)	⬇️
instrumentation/net/http/otelhttp/handler.go	72.6% <75.0%> (ø)
...entation/github.com/go-kit/kit/otelkit/endpoint.go	85.3% <85.3%> (ø)
...ion/google.golang.org/grpc/otelgrpc/interceptor.go	87.0% <87.5%> (ø)
exporters/metric/cortex/cortex.go	69.8% <94.4%> (ø)
detectors/aws/aws.go	84.6% <100.0%> (ø)
... and 24 more

[MrAlias]

Overall this looks good, thanks for the contribution. The blocking issues are the dependabot config change, upgrading to v0.18.0, and the RecordError question needs to be addressed.

[sagikazarmark]

@MrAlias thanks for the review. I addressed most of it.

Regarding RecordError: it sets the status for the span and technically it's not correct for the intermediate errors. I guess we can use it since the status will be set by the final error anyway. I think the behavior of RecordError was somewhat different when I first submitted this PR, so there might have been some other reason as well.

[sagikazarmark]

I changed it to use RecordError. Not fully convinced this is right though. I think semantically RecordError should only be called once for the terminal error (given it actually sets the status to error which might not always be desirable), but I could be wrong.

[MrAlias]

I changed it to use RecordError. Not fully convinced this is right though. I think semantically RecordError should only be called once for the terminal error (given it actually sets the status to error which might not always be desirable), but I could be wrong.

This might be something that needs to be clarified better. You are correct that the semantics of RecordError are hard to distinguish if it should be used here. Ultimately, the user of this instrumentation will want to send this telemetry to a backend that will need to understand the events that it receives. Sending these errors as events that are semantically tied to errors will provide the backend a chance to highlight these in helpful manner.

That said, OTel in general wanted to provide a better errors interface in the future so this is likely going to be improved.

Thanks for the contribution and patience!

[sagikazarmark]

Thanks for merging