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

Define an ownership model for packages that live in this repository #2834

Closed
MrAlias opened this issue Oct 4, 2022 · 19 comments · Fixed by #5555
Closed

Define an ownership model for packages that live in this repository #2834

MrAlias opened this issue Oct 4, 2022 · 19 comments · Fixed by #5555
Assignees
Milestone

Comments

@MrAlias
Copy link
Contributor

MrAlias commented Oct 4, 2022

This is a needed action for open-telemetry/opentelemetry-go-instrumentation#3 to be hosted here.

@pellared
Copy link
Member

pellared commented Oct 4, 2022

This may be helpful open-telemetry/opentelemetry-dotnet-instrumentation#1275 for setting up default reviewers

@Aneurysm9
Copy link
Member

I thought we were going to use a separate repo for this? Why the change?

@pdelewski
Copy link
Member

pdelewski commented Oct 4, 2022

There is an argument that contrib is a collection of tools and libraries used by people that want to use/learn about opentelemetry and may fit better to this kind of tool as it works on source level instead of runtime, however it depends on point of view as the only difference is point of hooking (source vs runtime). For me, there are arguments to add it to contrib, but there also arguments to add it to go-instrumentation

@MrAlias
Copy link
Contributor Author

MrAlias commented Oct 18, 2022

  • Codeowner file will need to be updated
    • Every Go module will need one or more unique user to be defined as an owner
    • The initial PR adding a module is required to also update the CODEOWNERS file with the explicit owners
    • Every go module will also need to have the approvers of the project listed as owners
  • Repo permissions need to be updated
    • Every PR in a sub-dir will need approval from one of the specifically defined owner user
    • [stretch goal] Have the owners be able to merge PR
  • Deprecation strategy
    • If the unique owners of the sub-dir are not able to continue on as owners (this need to be better defined) they are removed from the CODEOWNERS file
    • If there are not unique owners of a sub-dir, that modules is deprecated

Work to be done to adopt this:

  • All existing modules need to be allocated to a owner or deprecated.
  • Add this all as a policy to the CONTRIBUTING.md doc.

@MrAlias
Copy link
Contributor Author

MrAlias commented Oct 20, 2022

An approver/maintainer from this project would need to be a CODEOWNER of each module as well.

@MrAlias
Copy link
Contributor Author

MrAlias commented Oct 20, 2022

In collector contrib there is a labeling model that helps alert CODEOWNERS when PRs/Issues are added.

@MrAlias
Copy link
Contributor Author

MrAlias commented Nov 29, 2022

TODO:

  • Inventory and list all Go modules in this repo in this issue.
  • From inventory get buy-in from potential owners (maintainer and domain expert)
    • 2 week cycle
  • Deprecate all modules that do not have owners.
  • Modules that do have owners, updated the CODEOWNERS file to identify this.

@MadVikingGod
Copy link
Contributor

MadVikingGod commented Nov 29, 2022

The inventory:

  • * (The root, this should be the approvers)
  • detectors/aws/ (or the following 4)
  • detectors/aws/ec2/
  • detectors/aws/ecs/
  • detectors/aws/eks/
  • detectors/aws/lambda/
  • detectors/gcp/
  • instrumentation/github.com/astaxie/beego/otelbeego/
  • instrumentation/github.com/aws/aws-lambda-go/otellambda/
  • instrumentation/github.com/aws/aws-sdk-go-v2/otelaws/
  • instrumentation/github.com/bradfitz/gomemcache/memcache/otelmemcache/
  • instrumentation/github.com/emicklei/go-restful/otelrestful/
  • instrumentation/github.com/gin-gonic/gin/otelgin/
  • instrumentation/github.com/go-kit/kit/otelkit/
  • instrumentation/github.com/gocql/gocql/otelgocql/
  • instrumentation/github.com/gorilla/mux/otelmux/
  • instrumentation/github.com/labstack/echo/otelecho/
  • instrumentation/github.com/Shopify/sarama/otelsarama/
  • instrumentation/go.mongodb.org/mongo-driver/mongo/otelmongo/
  • instrumentation/google.golang.org/grpc/otelgrpc/
  • instrumentation/gopkg.in/macaron.v1/otelmacaron/
  • instrumentation/host/
  • instrumentation/net/http/httptrace/otelhttptrace/
  • instrumentation/net/http/otelhttp/
  • instrumentation/runtime/
  • propagators/autoprop/
  • propagators/aws/
  • propagators/b3/
  • propagators/jaeger/
  • propagators/opencensus/
  • propagators/ot/
  • samplers/aws/xray/
  • samplers/jaegerremote/
  • samplers/probability/consistent/
  • tools/ (this could fall under the top level)
  • zpages/

@MrAlias
Copy link
Contributor Author

MrAlias commented Nov 29, 2022

@open-telemetry/go-approvers please comment here if you would be willing to take on the ownership responsibility for an above module.

This will include you being required to approve PRs for that module and have the ultimate responsibility for an issue related to the module (you are not expected to complete all work there, just you will be expected to keep development moving at a reasonable pace.).

@dashpole
Copy link
Contributor

I am willing to take detectors/gcp/, propagators/opencensus/, and instrumentation/google.golang.org/grpc/otelgrpc/ if needed.

@pellared
Copy link
Member

I can take propagators/b3/ and instrumentation/github.com/Shopify/sarama/otelsarama/

@MadVikingGod
Copy link
Contributor

Before too many more people chime in. There is the initial codeowners file in #3045. You can add yourself as a change request to that PR, or wait until after it's merged and open a PR to accept ownership.

@MrAlias
Copy link
Contributor Author

MrAlias commented Dec 1, 2022

@pellared
Copy link
Member

pellared commented Aug 9, 2023

@yurishkuro propagators/jaeger and samplers/jaegerremote are not maintained. Are you OK of dropping support for them or would you find someone who could be a code owner of these modules?

@pellared
Copy link
Member

pellared commented Aug 9, 2023

@MrAlias propagators/ot is used by propagators/autoprop. I think it would be better to keep propagators/ot. Would you be able to be a code owner of this module?

@pellared
Copy link
Member

pellared commented Aug 9, 2023

otelmongo is a quite popular instrumentation library and we should ask if it could be moved to https://github.com/mongodb (similarly like for otelsarama). I decided to postpone the deprecation. I created https://jira.mongodb.org/browse/GODRIVER-2938

@pellared
Copy link
Member

pellared commented Aug 9, 2023

For HTTP instrumentation libraries that lack owners, I think we should simply improve otelhttp so that it can be reused with other libraries.

The question is if we should improve otelhttp before deprecating the libraries to provide an migration path for the users? Some part of it is in my head after looking at #4018, #4017. For now, I decided to postpone the deprecation.

@yurishkuro
Copy link
Member

@yurishkuro propagators/jaeger and samplers/jaegerremote are not maintained. Are you OK of dropping support for them or would you find someone who could be a code owner of these modules?

What does dropping support mean?

propagators/jaeger: I expect this to be a very stable code that does not require maintenance (it's not like the format is changing). But support for the format is required by the Spec: https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/context/api-propagators.md#propagators-distribution

samplers/jaegerremote: I'm ok to own it. It is also in the spec: https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/trace/sdk.md#jaegerremotesampler

@pellared
Copy link
Member

pellared commented Aug 10, 2023

@yurishkuro propagators/jaeger and samplers/jaegerremote are not maintained. Are you OK of dropping support for them or would you find someone who could be a code owner of these modules?

What does dropping support mean?

Deprecation in next release. Removal from codebase and OTel registry in subsequent release.

propagators/jaeger: I expect this to be a very stable code that does not require maintenance (it's not like the format is changing). But support for the format is required by the Spec: https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/context/api-propagators.md#propagators-distribution

samplers/jaegerremote: I'm ok to own it. It is also in the spec: https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/trace/sdk.md#jaegerremotesampler

Then I will propose a PR to add you as code owner for both modules. Please approve them if you are OK with it and thanks for the collaboration ❤️

EDIT: PTAL

@pellared pellared added this to the untracked milestone Nov 25, 2024
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 a pull request may close this issue.

7 participants