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

Deprecate and remove go.opentelemetry.io/contrib/instrumentation/github.com/gin-gonic/gin/otelgin #6190

Closed
pellared opened this issue Oct 4, 2024 · 8 comments · Fixed by #6370
Labels
abandoned Relates to a module that has been abandoned and is scheduled for deprecation and removal instrumentation: otelgin

Comments

@pellared
Copy link
Member

pellared commented Oct 4, 2024

This module has been identified to not have an owner. Based on the project's ownership policy, this module will be deprecated and then removed.

How to keep this module

For this module to continue in this repository, it needs a sponsor.

If you would like to sponsor this module and become an owner, please comment in this issue about your desire. As an owner you will assume the following responsibilities:

  • You will need to be a member of the OpenTelemetry organization and maintain that membership.
    • If you are not an existing member, we can add you to the community. This is not a disqualification, but something you will need in the process of becoming a sponsor.
  • You will be responsible for keeping up with the instrumented library. Any "upstream" changes that impact this module need to be proactively handle by you.
  • You will be expected to review any Pull Requests or Issues created that relate to this module.
  • You will be responsible for the stability and versioning compliance of the module.

You will need to have a good working knowledge of the code this module is instrumenting and, ideally, familiarity with the existing module code.

How this module will be removed

This module is in the process of being deprecated. After that deprecation notice has been published, we will wait 3 months or 2 two releases (whichever is the longer time period). After that time period, this module will be removed from this repository and no more versions of the module will be published.

Resurrection

If a sponsor is found after the module has been deprecated or removed, these operations can be reversed (i.e. coded added back, deprecation notice removed).

@abiabsurd
Copy link

abiabsurd commented Oct 17, 2024

I have a proposal to replace the existing implementation of otelgin with a relatively simple adapter package — which I've already written and am using in a private codebase — that enables usage of the otelhttp package as a gin middleware. Essentially the proposal would be to roughly match the existing func signature

package otelgin

func Middleware(service string, opts ...otelhttp.Option) gin.HandlerFunc {

where this new implementation uses otelhttp.Middleware under the hood.

In addition to addressing the root of this github issue, the solution also provides metrics instrumentation, which addresses several other open issues (example).

How do you recommend I proceed @pellared?

@dmathieu
Copy link
Member

This kind of refactoring would provide breaking changes to the library.
So it would have to be a new library, not just a commit in the existing and deprecated one.

Instrumentations don't necessarily have to be within this repository. You can absolutely host one on your own, provide documentation for it, and add it to the otel registry.

@pellared
Copy link
Member Author

pellared commented Oct 17, 2024

@abiabsurd, in addition to what @dmathieu wrote:

I think that such approach would only make sense only if all other HTTP instrumentation libraries (like otelmux, otelecho) would be designed in similar manner.

To move this forward you would need present this design (e.g. by sharing some design doc, by providing draft PR or linking to existing repositories, discussing it during a SIG meeting). Additionally you would have to agree to become a code owner if we would accept such approach. Maybe you want to help with Go: HTTP Semconv Migration?

@abiabsurd
Copy link

This kind of refactoring would provide breaking changes to the library. So it would have to be a new library, not just a commit in the existing and deprecated one.

Instrumentations don't necessarily have to be within this repository. You can absolutely host one on your own, provide documentation for it, and add it to the otel registry.

I was assuming it would just be a new major version of the library to indicate the breaking change, but I'm also open to hosting it myself if that's a preferable approach.

@abiabsurd
Copy link

I think that such approach would only make sense only if all other HTTP instrumentation libraries (like otelmux, otelecho) would be designed in similar manner.

Understood. I'm not interested in refactoring those libraries at this current time, so I think it makes sense for me to proceed with what @dmathieu suggested. Thank you.

@flc1125
Copy link
Member

flc1125 commented Nov 22, 2024

@pellared @dmathieu

I have conducted some initial code reviews based on otelgin and checked each issue reported in the repository. Below are my current thoughts:

Step 1: I need to understand and become more familiar with the existing logic of otelgin and each line of code so that I can clearly identify the package's logic, potential optimization points, enhancement opportunities, and even defects. To achieve this, I plan to do the following:

  • I will perform code optimizations and appropriate refactoring on the existing code while keeping the original functionality intact and ensuring backward compatibility.
  • This process may involve several small iterations.
  • Additionally, I will strive to improve the coverage of unit tests to achieve over 90% wherever possible.

Step 2: Address the issues reflected in the existing feedback.

  • This step may be conducted in parallel with the first step, depending on the actual situation.
  • However, the emphasis will likely remain on the first step. Therefore, the resolution of related issues may be somewhat delayed.

Step 3: Attempt to introduce and enhance some new functional features to meet the community's usage needs.

  • For example, known support for features like Metrics, etc.

Step 4: Attempt to expand into other components based on ginotel.

  • These components may include others similar to otelecho, but that will likely be a longer-term goal.
  • I need to stay focused and address the issues with otelgin first.

The above are my thoughts. If there are any other good suggestions, please let me know.

@dmathieu dmathieu linked a pull request Nov 26, 2024 that will close this issue
dmathieu added a commit that referenced this issue Nov 27, 2024
Added myself (akats7) as codeowner for otelgin to avoid deprecation
#6190

Co-authored-by: Damien Mathieu <[email protected]>
@flc1125
Copy link
Member

flc1125 commented Nov 28, 2024

Although it has been closed, can I still apply to be the maintainer of otelgin?

@dmathieu
Copy link
Member

Yes, of course. This issue being closed only means the component won't be deprecated.

@MrAlias MrAlias modified the milestone: v1.33.0 Dec 11, 2024
@MrAlias MrAlias closed this as not planned Won't fix, can't repro, duplicate, stale Dec 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
abandoned Relates to a module that has been abandoned and is scheduled for deprecation and removal instrumentation: otelgin
Projects
Development

Successfully merging a pull request may close this issue.

5 participants