Initial gin-gonic instrumentation with otel-go #15

krnowak · 2020-04-16T16:24:25Z

This is some initial code that does the instrumentation. Still need to fix the tests, add docs and a Dockerfile with an example app. So it's work in progress.

There is the common.go file that is mostly about attributes for the span. I suppose it will end up being elsewhere in the long run.

plugins/gin-gonic/gin/common.go

plugins/gin-gonic/gin/example_test.go

krnowak · 2020-04-21T12:50:59Z

Ready for review. For now I have added a mock tracer to otel-go-contrib instead of exposing the one in otel-go. This is to avoid waiting for the potential otel-go changes to make it to master.

andrewhsu

LGTM, glad to see the Dockerfile and docker-compose. I'll have a go at the docker-compose later to verify.

lizthegrey · 2020-04-21T18:13:14Z

Didn't Datadog sign the CLA allowing the copyright notice to read just the OTel authors?

krnowak · 2020-04-22T14:05:17Z

Updated:

plugin/gin-gonic/gin/common{,_test}.go are now internal/trace/http{,_test}.go
fixed an import path comment in plugin/gin-gonic/gin/doc.go to use the vanity path

Also me when you talk about legal stuff: [insert meme with Homer Simpson backing into the bushes] ;)

krnowak · 2020-04-22T18:26:57Z

Updated again to fix a documentation of the WithTracer option.

andrewhsu · 2020-04-22T19:48:51Z

Didn't Datadog sign the CLA allowing the copyright notice to read just the OTel authors?

@lizthegrey i don't believe anybody from datadog had committed to this repo with a signed CLA as was done in the opentelemetry-python-contrib repo open-telemetry/opentelemetry-python-contrib#1

andrewhsu

requesting change as described in #15 (comment)

krnowak · 2020-04-22T20:09:15Z

Updated, did some small fixes in tests, and added the NOTICE file.

lizthegrey

I'm sorry, but I'm going to need to block merging this until we can discuss the CLA, the copyright notice, etc. at governance committee.

andrewhsu · 2020-04-22T21:04:43Z

I'm sorry, but I'm going to need to block merging this until we can discuss the CLA, the copyright notice, etc. at governance committee.

@lizthegrey is there an eta on discussing with the governance committee?

fyi i used https://github.com/cncf/foundation/blob/master/copyright-notices.md#what-about-third-party-code as guidance for this PR since the code is altered from original source

lizthegrey · 2020-04-22T21:10:52Z

I'm sorry, but I'm going to need to block merging this until we can discuss the CLA, the copyright notice, etc. at governance committee.

@lizthegrey is there an eta on discussing with the governance committee?

Tomorrow.

fyi i used cncf/foundation:copyright-notices.md@master#what-about-third-party-code as guidance for this PR since the code is altered from original source

There are certain sensitivities here about not wanting to bias towards any one vendor in the copyright notices, etc. -- none of the other code contributed to OTel says "copyright Honeycomb", "copyright Lightstep", etc. so it's a matter of fairness.

lizthegrey · 2020-04-23T17:38:56Z

I'm sorry, but I'm going to need to block merging this until we can discuss the CLA, the copyright notice, etc. at governance committee.

@lizthegrey is there an eta on discussing with the governance committee?

Tomorrow.

fyi i used cncf/foundation:copyright-notices.md@master#what-about-third-party-code as guidance for this PR since the code is altered from original source

There are certain sensitivities here about not wanting to bias towards any one vendor in the copyright notices, etc. -- none of the other code contributed to OTel says "copyright Honeycomb", "copyright Lightstep", etc. so it's a matter of fairness.

We met. @tedsuo will follow up with more specific detail, but the CNCF third party code guideline applies unless the contributor has waived the requirement. In this case, the contributor Datadog has waived the requirement (they are not a third party to this project). One way to move forward may be to explicitly get a Datadog engineer to append the commit that strips the copyright notices to make clear that it's officially sanctioned.

tedsuo · 2020-04-24T16:20:52Z

Thanks @lizthegrey. I'm reaching out to Datadog. I feel we have been given permission to change these licenses, but I will confirm again, and look to adding a clause in our community repo to clarify this subject.

MikeGoldsmith

Plugin code looks good, I understand there is some stuff to iron out regarding the DataDog licence though.

jeremy-lq · 2020-05-13T20:41:57Z

Datadog did give our explicit permission to attribute copyright to "OpenTelemetry Authors" here: open-telemetry/community#305 (comment)

I wasn't previously aware of this thread. If there are future questions about the topic or clarification needed, do feel free to mention me so I can take a look.

lizthegrey · 2020-05-13T20:49:20Z

Thanks, y'all.

Based on [this](open-telemetry/community#305 (comment)) approval, changing attribution to the OpenTelemetry authors.

Use new `kv` and `value` packages.

andrewhsu

LGTM

Based on her "Thanks y'all"

goimports for import rewritting golangci-lint as the configurable linting swiss army knife. These tools are recorded in [tools.go](https://github.com/golang/go/wiki/Modules#how-can-i-track-tool-dependencies-for-a-module). This records them as a dependency to make sure we're all using the same tool versions. To make sure this project's tool's versions don't stomp over versions from other projects, they are installed in ./.tools, which is .gitignored. goimports was run and fixed up a single file: plugin/httptrace/httptrace.go I prefer to group local packages below external packages, hence the use of goimports -local option. .golangci.yml contains nothing but an incomplete set of defaults ATM. I expect those to change over time though. ;-) To use, run: $ make precommit Fixes #15

krnowak requested review from jmacd, lizthegrey, MrAlias, paivagustavo, rghetia and tedsuo as code owners April 16, 2020 16:24

jmacd reviewed Apr 16, 2020

View reviewed changes

plugins/gin-gonic/gin/common.go Outdated Show resolved Hide resolved

jmacd approved these changes Apr 17, 2020

View reviewed changes

krnowak force-pushed the gin-gonic branch from 76e86e5 to 26244b9 Compare April 18, 2020 16:50

MrAlias reviewed Apr 20, 2020

View reviewed changes

plugins/gin-gonic/gin/example_test.go Outdated Show resolved Hide resolved

krnowak force-pushed the gin-gonic branch from 0de5820 to e5a9fc2 Compare April 21, 2020 12:22

krnowak changed the title ~~WIP: Initial gin-gonic instrumentation with otel-go~~ Initial gin-gonic instrumentation with otel-go Apr 21, 2020

andrewhsu approved these changes Apr 21, 2020

View reviewed changes

krnowak force-pushed the gin-gonic branch from e5a9fc2 to 5129112 Compare April 22, 2020 13:57

krnowak force-pushed the gin-gonic branch from 5129112 to b079a7b Compare April 22, 2020 18:26

krnowak force-pushed the gin-gonic branch from b079a7b to 767be6d Compare April 22, 2020 19:39

andrewhsu suggested changes Apr 22, 2020

View reviewed changes

krnowak force-pushed the gin-gonic branch from 767be6d to 5afc2cd Compare April 22, 2020 20:07

lizthegrey previously requested changes Apr 22, 2020

View reviewed changes

krnowak mentioned this pull request Apr 22, 2020

gorilla/mux instrumentation #19

Merged

MikeGoldsmith mentioned this pull request Apr 24, 2020

Add Macaron plugin #20

Merged

MikeGoldsmith approved these changes Apr 25, 2020

View reviewed changes

MrAlias mentioned this pull request May 1, 2020

Port mongodb driver instrumentation #26

Merged

lizthegrey added the blocked: copyright Waiting on licensing to be addressed before progress can be made label May 4, 2020

andrewhsu mentioned this pull request May 5, 2020

Can we remove the requirement for license headers? open-telemetry/community#113

Closed

krnowak force-pushed the gin-gonic branch from 5afc2cd to 443cf48 Compare May 5, 2020 15:18

krnowak requested a review from Aneurysm9 as a code owner May 5, 2020 15:18

krnowak added 2 commits May 5, 2020 19:22

Add an instrumentation for gin-gonic web framework

f9b37a1

Add an example use of the gin-gonic instrumentation

eb24514

krnowak force-pushed the gin-gonic branch from 443cf48 to eb24514 Compare May 5, 2020 17:23

MrAlias self-requested a review May 14, 2020 19:52

Update attribution and remove NOTICE

13440f4

Based on [this](open-telemetry/community#305 (comment)) approval, changing attribution to the OpenTelemetry authors.

krnowak requested a review from evantorrie as a code owner May 14, 2020 20:04

Merge branch 'master' into krnowak/gin-gonic

78e7640

MrAlias requested a review from lizthegrey May 14, 2020 20:07

MrAlias removed the blocked: copyright Waiting on licensing to be addressed before progress can be made label May 14, 2020

Update to v0.5.0 release

fd99aa9

Use new `kv` and `value` packages.

MrAlias approved these changes May 14, 2020

View reviewed changes

andrewhsu approved these changes May 14, 2020

View reviewed changes

jmacd merged commit 87b1c69 into open-telemetry:master May 14, 2020

This was referenced Jun 2, 2020

add Gin plugin for Golang open-telemetry/opentelemetry.io#188

Merged

add support for contrib plugin gin 🍸 #6

Closed

pellared added this to the untracked milestone Nov 8, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial gin-gonic instrumentation with otel-go #15

Initial gin-gonic instrumentation with otel-go #15

krnowak commented Apr 16, 2020

krnowak commented Apr 21, 2020

andrewhsu left a comment

lizthegrey commented Apr 21, 2020

krnowak commented Apr 22, 2020 •

edited

Loading

krnowak commented Apr 22, 2020

andrewhsu commented Apr 22, 2020

andrewhsu left a comment

krnowak commented Apr 22, 2020

lizthegrey left a comment

andrewhsu commented Apr 22, 2020

lizthegrey commented Apr 22, 2020

lizthegrey commented Apr 23, 2020 •

edited

Loading

tedsuo commented Apr 24, 2020

MikeGoldsmith left a comment

jeremy-lq commented May 13, 2020

lizthegrey commented May 13, 2020

andrewhsu left a comment

Initial gin-gonic instrumentation with otel-go #15

Initial gin-gonic instrumentation with otel-go #15

Conversation

krnowak commented Apr 16, 2020

krnowak commented Apr 21, 2020

andrewhsu left a comment

Choose a reason for hiding this comment

lizthegrey commented Apr 21, 2020

krnowak commented Apr 22, 2020 • edited Loading

krnowak commented Apr 22, 2020

andrewhsu commented Apr 22, 2020

andrewhsu left a comment

Choose a reason for hiding this comment

krnowak commented Apr 22, 2020

lizthegrey left a comment

Choose a reason for hiding this comment

andrewhsu commented Apr 22, 2020

lizthegrey commented Apr 22, 2020

lizthegrey commented Apr 23, 2020 • edited Loading

tedsuo commented Apr 24, 2020

MikeGoldsmith left a comment

Choose a reason for hiding this comment

jeremy-lq commented May 13, 2020

lizthegrey commented May 13, 2020

andrewhsu left a comment

Choose a reason for hiding this comment

krnowak commented Apr 22, 2020 •

edited

Loading

lizthegrey commented Apr 23, 2020 •

edited

Loading