Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add opentelemetry support for gorm2 #835

Closed
wants to merge 2 commits into from
Closed

add opentelemetry support for gorm2 #835

wants to merge 2 commits into from

Conversation

wei840222
Copy link

Here is instrumentation for https://github.com/go-gorm/gorm.
A popular SQL database ORM framework.
Include README.md for how to used.
It seems need some test for this lib, it will be processed later.

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Jun 28, 2021

CLA Signed

The committers are authorized under a signed CLA.

@vadimberezniker
Copy link
Contributor

I am looking at adding GORM instrumentation to our service and came across this PR.
I see that this is derived from https://github.com/go-gorm/opentracing. Have you reached out the GORM maintainers to ask if they would prefer that this be maintained as part of GORM rather as part of the otel repo?
The other observation is that https://github.com/go-gorm/opentracing is MIT licensed and this PR is stripping the copyright & license.

Copy link
Member

@Aneurysm9 Aneurysm9 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the feedback @vadimberezniker. I think our general preference is to have first-party instrumentation supported by library authors. If the GORM maintainers wanted to support this instrumentation that would be great and I'd be happy to help them in any way possible.

It is concerning that this contribution appears to be derived from the existing GORM OpenTracing instrumentation without attribution or appropriate license provenance. I'm not sure that we can accept this contribution or anything substantially similar to it.

@MrAlias
Copy link
Contributor

MrAlias commented Jul 1, 2021

Thanks for the feedback @vadimberezniker. I think our general preference is to have first-party instrumentation supported by library authors. If the GORM maintainers wanted to support this instrumentation that would be great and I'd be happy to help them in any way possible.

It is concerning that this contribution appears to be derived from the existing GORM OpenTracing instrumentation without attribution or appropriate license provenance. I'm not sure that we can accept this contribution or anything substantially similar to it.

Agreed. Thanks for the contribution, but this needs more strategy and community involvement before we are ready to accept this. Please open an "Instrumentation Request" type Issue where the solution can be discussed. I would, similar to others, prefer the instrumentation live natively in the library that is being instrumented. But am open to more discussion on the topic in the Issue if there is disagreement.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants