Remove OTEL/contrib dependency on cmd/collector/app/sampling

[yurishkuro]

Part of #6408, please read that first.

The dependency exists in OTEL/contrib/extension/jaegerremotesampling. It might be relatively easy to break, but need to try it out.

• Copy whichever code is needed from cmd/collector/app/sampling and plugin/sampling/strategyprovider/static to OTEL/contrib.
• Refactor Jaeger code internally so that these modules are hidden under some /internal/ package (discuss options/proposals on this ticket first)

[VaibhavMalik4187]

Hi @yurishkuro, I'm interested in trying this. I'll try to prepare a doc to discuss this in detail.

[ary82]

Hi @yurishkuro, for the first part of this issue, here are my findings:

In OTEL/contrib/extension/jaegerremotesampling,

1. The internal package has the following dependencies:

   • Provider interface from cmd/collector/app/sampling/samplingstrategy
   • GRPCHandler from cmd/collector/app/sampling

2. As for the root package in OTEL/contrib/extension/jaegerremotesampling, it depends on the code from:

   • plugin/sampling/strategyprovider/static/constants.go
   • plugin/sampling/strategyprovider/static/options.go
   • plugin/sampling/strategyprovider/static/providers.go

Also, some of these have a dependency on github.com/jaegertracing/jaeger/proto-gen/api_v2, which I can see in the parent issue #6408 is also marked for moving. For this issue, do we need to resolve this dependency on the copied code or should we add it as is?

[yurishkuro]

@ary82 I would keep the dependency on proto-gen/api_v2 for now, to reduce the scope. Once the other dependencies you mentioned are untangled it would be easier to refactor them to not depend on proto-gen from Jaeger (they can generate the same code internally).

[ary82]

Hi @yurishkuro, I'm still learning jaeger's architecture and the code structure, but here's what I think for the second part of this issue:

• As cmd/collector/app/sampling contains the models, interface and factory for sampling strategy components, as well as the GRPCHandler. The structs in model are used by various storage components too. How about moving them to a new package cmd/jaeger/internal/sampling.

• plugin/sampling/strategyprovider defines the implementations of sampling strategies, namely static and adaptive. We can move these to a package inside cmd/jaeger/internal/extension/remotesampling

What do you think?

[yurishkuro]

I don't think you can move stuff into cmd/jaeger/internal/sampling because it's also being used from cmd/collector and it will not be able to access internal from there.

[ary82]

Took a closer look at the dependencies. Currently, cmd/collector/app/sampling components are being used in:

• cmd/collector
• cmd/jaeger
• storage/samplingstore
• plugin/storage components
• Their implementations, adaptive and static, currently in plugin/sampling/strategyprovider (subject to change as part of this issue)
• pkg/clientcfg

Seeing as they have a lot of components depending upon them, would it make more sense in moving these to the root internal?

As for plugin/sampling/strategyprovider components, they're used by:

• cmd/jaeger
• cmd/collector
• cmd/internal

So I think we might benefit from moving them to cmd/internal. Does this make sense?

[yurishkuro]

all the sampling components are effectively internal services, so I would move them to internal/sampling. Can you propose the sub-structure for that, with explanations what each sub-folder will contain?

[ary82]

This maintains the current package hierarchy for the most part, and keeps the samplingstrategy interfaces in the internal/sampling package. Am I on the right track?

└── internal
    └── sampling
        ├── model/ - contains the models required by various components
        ├── grpc/ - strategy handler for grpc
        ├── leaderelection/
        ├── strategyprovider
        │   ├── static/ - static strategy store
        │   ├── adaptive/ - adaptive strategy store
        │   │   └── calculationstrategy/ - calculation for adaptive strategy
        │   ├── factory.go - implementation of factory interface as meta-factory
        │   └── factory_config.go - config for meta-factory
        ├── factory.go - interface for factory
        └── interface.go - provider & aggregator interface

[yurishkuro]

a few comments

• model - is it auto-generated from Protobuf? Where is it now?
• leaderelection is not really just a sampling concept. Is it just an interface definition? If so I'd put it into root/internal/leaderelection
• grpc - don't we also have http handlers?
• static - we should rename it to file, to match jaeger-v2 nomenclature
• factory.go and interface.go - factory for what?
• where will cmd/collector/app/sampling/samplingstrategy go?

[ary82]

• model is currently at cmd/app/sampling/model. It is not autogenerated, it involves some types like Throughput and ServiceOperationData, used in adaptive strategyprovider and varous plugin/storage components.
• leaderelection contains the ElectionParticipant interface as well as its implementation DistributedElectionParticipant. This is currently only used by the adaptive sampling strategyprovider and called in cmd/jaeger/internal/extension/remotesampling.
• The http handler is defined in pkg/clientcfg/clientcfghttp package. We can also move it to internal/sampling.
• cmd/collector/app/sampling/samplingstrategy contains three interfaces:
  • Provider keeps track of Sampling strategies
  • Aggregator is used to record Throughput that an operation receives
  • Factory can create implementation of Provider and Aggregator. This is implemented in the meta-factory from strategyprovider as well as the individual implementations of static(file) and adaptive.
• I initially kept samplingstrategy in the internal/sampling root. Would keeping these in internal/sampling/samplingstrategy be a better structure?

How does this sound?

└── internal
    ├── leaderelection/
    └── sampling
        ├── model/ - contains some types used in adaptive sampling
        ├── grpc/ - strategy handler for grpc
        ├── http/ - moved from pkg/clientcfg/clientcfghttp
        ├── strategyprovider
        │   ├── file/
        │   ├── adaptive/
        │   │   └── calculationstrategy/
        │   ├── factory.go - implementation of samplingstrategy.factory interface as meta-factory
        │   └── factory_config.go - config for meta-factory
        └── samplingstrategy
            ├── factory.go - factory creates implementation of Provider and Aggregator
            └── interface.go - Provider & Aggregator interface

[yurishkuro]

looks better, but I would still like to simplify this more

• model/ seems quite useless package. It has some types that are only used in the context of sampling storage, so could we move them next to sampling storage interface? It also has types that are only used in private methods in plugin/storage/cassandra/samplingstore/storage.go and shouldn't be exposed at all in public.
• samplingstrategy - can we not merge it into strategyprovider? Where else are those implemented?

[ary82]

• Yeah, we can move the specific model/ types to plugin/storage/cassandra/samplingstore/storage.go and keep them private.
• The other types (Throughput, ServiceOperationProbabilities and ServiceOperationQPS) can be moved to the storage/samplingstore package.
• samplingstrategy interfaces are implemented as:
  • strategyprovider implements Factory as a meta-factory
  • file and adaptive implements Factory, Provider and Aggregator

Merging samplingstrategy components with strategyprovider will cause a circular dependency with file and adaptive, so we would need two different packages

[yurishkuro]

please elaborate why it will cause a circular dependency? On which types?

For example, if the dependency is on the interfaces that the structs in strategyprovider implement then it's possible to work around that - don't return interfaces from constructor functions, return a pointer to a public struct, and have a test file with strategyprovider_test package that verifies interface adherence.

[ary82]

That's right, the dependency is on the interfaces that the structs in strategyprovider implement. We can do this in a test file as you recommended. The refactor would look like:

├── storage
│   └── samplingstore
│       └── model/ - some types from `cmd/collector/sampling/model` moved here
└── internal
    ├── leaderelection/
    └── sampling
        ├── grpc/ - strategy handler for grpc
        ├── http/ - moved from pkg/clientcfg/clientcfghttp
        └── strategyprovider/
            ├── file/
            ├── adaptive/
            ├── metafactory.go - implementation of factory interface as meta-factory
            ├── metafactory_config.go - config for meta-factory
            ├── factory.go - factory interface creates implementation of Provider and Aggregator
            └── interface.go - Provider & Aggregator interface

Does this look better?

[yurishkuro]

looks great. Are you thinking of doing this all at once or incrementally by package? I'd prefer smaller PRs, as the whole thing could easily include 100 files.

[ary82]

Sounds good, I'll break it down:

• move cmd/collector/app/sampling/model
• move sampling grpc to internal/sampling/grpc
• move pkg/clientcfg/clientcfghttp to internal/sampling/http
• move plugin/sampling/leaderelection to internal/leaderelection
• move cmd/collector/app/sampling/samplingstrategy to internal/sampling/strategyprovider
• move plugin/sampling/strategyprovider to internal/sampling/strategyprovider