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

Organize the code repository to accommodate v2 #2837

Closed
jpkrohling opened this issue Feb 25, 2021 · 11 comments · Fixed by #2880
Closed

Organize the code repository to accommodate v2 #2837

jpkrohling opened this issue Feb 25, 2021 · 11 comments · Fixed by #2880
Assignees

Comments

@jpkrohling
Copy link
Contributor

We talked before about reorganizing the code repository so that we can have both the current code plus the current contents of jaeger-otelcol in one place.

This task is to track the proposal and implementation for this.

@jpkrohling jpkrohling added this to the Release 2.0 milestone Feb 25, 2021
@jpkrohling
Copy link
Contributor Author

@joe-elliott Did you volunteer for this in the past? Are you still interested in working on this?

@joe-elliott
Copy link
Member

Yup, I can spend some time on this. I was thinking:

./opentelemetry
   /manifests
   /processors
   /exporters
   /receivers

Each processor/exporter/receiver would have a go.mod allowing it to be referenced by the otelcol-builder. Then we could add makefile entries for calling the otelcol-builder like we have in jaeger-otelcol.

@jpkrohling
Copy link
Contributor Author

jpkrohling commented Feb 25, 2021

For a v2 branch, if we decide to go this way, we could have something like this instead:

├── exporter
│   └── badger
├── extension
│   └── query
├── internal
├── manifests
├── pkg
│   ├── boundedqueue
│   ├── http
│   │   └── query
│   └── spi
│       └── storage
├── processor
└── receiver

Each "final'' package (pkg/boundedqueue/, pkg/spi/storage/, exporter/badger/) would be a go module.

About the v2 branch: we might need to have it anyway in order to satisfy Go requirements, no?

@yurishkuro
Copy link
Member

yurishkuro commented Feb 25, 2021

fwiw, I strongly prefer organizing code by business functions, not implementation details.

  • extension -> query is the inverse of that, I don't care how query is implemented, as extension or not.
  • processor & receiver are ok
  • exporter -> badger is a bit iffy, I'd say "storage" is a more important aspect of badger than "exporter". I would expect these at the lop level:exporter -> storage and storage -> badger, because storage is a much bigger API than exporter, and exporter does not really care which storage

@jpkrohling
Copy link
Contributor Author

True. From otelcol's perspective, extension, exporter, ... are part of its business if we consider it as an ingestion pipeline. For Jaeger, the business isn't just ingesting the data, so, it does make sense to organize it differently.

But then, what about pkg?

@yurishkuro
Copy link
Member

But then, what about pkg?

hehe, golang-standards/project-layout#10

I would keep pkg the same as it is in Jaeger today, a collection of standalone utils that could be theoretically moved out of the project. NOT storage APIs.

Also, in retrospect I think the current Jaeger structure could've been better if storage was all under storage/, instead of only APIs being there, but the impls being under plugin/storage/. It was unnecessary complication / separation.

@jpkrohling
Copy link
Contributor Author

Something like this then?

.
├── agent
│   └── jaegerthriftudp
├── collector
│   ├── jaegerproto
│   └── jaegerthrifthttp
├── internal
├── manifests
├── pkg
├── query
│   ├── extension
│   └── handler
└── storage
    ├── badger
    ├── cassandra
    ├── elasticsearch
    ├── grpcplugin
    └── spi

I would keep pkg the same as it is in Jaeger today, a collection of standalone utils that could be theoretically moved out of the project. NOT storage APIs.

Just to confirm: you don't mean the current set packages, just the general concept, right? Quite a few of those packages would probably not be useful for v2.

Also, did we settle in using a v2 branch vs. v2 directory in the main branch?

@yurishkuro
Copy link
Member

Something like this then...

Is each binary going to have its own go.mod? That approach really bothers me. I've worked with monorepos before, and they always had a single set of dependencies, which is kind of one of the main points of the monorepo.

Just to confirm: you don't mean the current set packages, just the general concept, right? Quite a few of those packages would probably not be useful for v2.

Yes, I am referring to the concept.

Also, did we settle in using a v2 branch vs. v2 directory in the main branch?

My preference is to keep everything in the main branch, refactor it as needed.

@jpkrohling
Copy link
Contributor Author

Is each binary going to have its own go.mod?

Each OpenTelemetry Collector component needs to be its own go mod. In the current proposal, the following would be otelcol components:

  • v2/agent/jaegerthriftudp/
  • v2/collector/jaegerproto/
  • v2/collector/jaegerthrifthttp/
  • v2/query/extension/
  • v2/remotesampling/extension
  • v2/storage/badger/
  • v2/storage/cassandra/
  • v2/storage/elasticsearch/
  • v2/storage/grpcplugin/

which is kind of one of the main points of the monorepo

The otelcol-contrib is indeed having problems with dependencies. This was kind of the reason for having jaeger-otelcol.

My preference is to keep everything in the main branch, refactor it as needed.

Alright, then:

.
└── v2
    ├── agent
    │   └── jaegerthriftudp
    ├── collector
    │   ├── jaegerproto
    │   └── jaegerthrifthttp
    ├── internal
    ├── manifests
    ├── pkg
    ├── query
    │   ├── extension
    │   └── handler
    ├── remotesampling
    │   ├── extension
    │   └── handler
    └── storage
        ├── badger
        ├── cassandra
        ├── elasticsearch
        ├── grpcplugin
        └── spi

@jpkrohling
Copy link
Contributor Author

jpkrohling commented Mar 2, 2021

Each OpenTelemetry Collector component needs to be its own go mod. In the current proposal, the following would be otelcol components:

Actually, it might be possible to change the builder to not require a go module... Let me scratch something (tracked here: open-telemetry/opentelemetry-collector-builder#12)

@jpkrohling
Copy link
Contributor Author

I just confirmed that it is possible to build a distribution with multiple components coming from the same module.

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.

3 participants