[jaeger-v2] Add remotesampling extension

[Pushkarm029]

Description of the changes

• adds static sampling support for otel-based jaeger-v2.
• supports hot reload
• added unit tests

How was this change tested?

• go run -tags=ui ./cmd/jaeger --config ./cmd/jaeger/config-badger.yaml
• make test

Checklist

• I have read https://github.com/jaegertracing/jaeger/blob/master/CONTRIBUTING_GUIDELINES.md
• I have signed all commits
• I have added unit tests for the new functionality
• I have run lint and test steps successfully
  • for jaeger: make lint test
  • for jaeger-ui: yarn lint and yarn test

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 41.12%. Comparing base (13cbaed) to head (076eb58).
Report is 5 commits behind head on main.

Additional details and impacted files

@@             Coverage Diff             @@
##             main    #5389       +/-   ##
===========================================
- Coverage   94.54%   41.12%   -53.42%     
===========================================
  Files         346      171      -175     
  Lines       16947     9091     -7856     
===========================================
- Hits        16022     3739    -12283     
- Misses        724     5018     +4294     
- Partials      201      334      +133     

Flag	Coverage Δ
badger_v1	10.28% <ø> (-0.01%)	⬇️
badger_v2	?
cassandra-3.x	?
cassandra-3.x-v1	18.16% <ø> (?)
cassandra-3.x-v2	6.11% <ø> (?)
cassandra-4.x	?
cassandra-4.x-v1	18.16% <ø> (?)
cassandra-4.x-v2	6.14% <ø> (?)
elasticsearch-5.x	5.73% <ø> (-0.18%)	⬇️
elasticsearch-6.x	5.73% <ø> (-0.17%)	⬇️
elasticsearch-7.x	5.74% <ø> (-0.16%)	⬇️
elasticsearch-8.x	5.74% <ø> (-0.16%)	⬇️
grpc_v1	12.59% <ø> (-0.04%)	⬇️
grpc_v2	?
kafka	9.99% <ø> (+0.02%)	⬆️
opensearch-1.x	5.73% <ø> (?)
opensearch-2.x	5.74% <ø> (-0.16%)	⬇️
unittests	?

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[Pushkarm029]

I am thinking of this approach.:

• create a gRPC sampling server
• then, register api_v2.RegisterSamplingManagerServer(server, sampling.NewGRPCHandler(SamplingStore))
• similarly, create an HTTP server and register a sampling API.

That's how it works in the current Jaeger collector, but we are serving this API in the collector server. Here, we have to create a new one.

suggestions

[Pushkarm029]

One more question:

How can I run hotrod with jaeger-v2 binary?
I tried this, but Jaeger UI was not able to detect Hotrod service.
OTEL_EXPORTER_OTLP_ENDPOINT=http://localhost:4318 go run ./main.go all
go run -tags=ui ./cmd/jaeger --config ./cmd/jaeger/config-elasticsearch.yaml

[yurishkuro]

We need a remotesampling extension which performs the following duties:

• creates HTTP server that returns sampling strategies as JSON
• allows user a choice of a sampling strategy store
  • file: a static file with auto-reload
  • adaptive: a backend storage that implements interfaces in sampling/strategystore package

We also need an adaptivesampling processor that

• uses remotesampling to obtain storage backend (similar how jaegerexporter uses jaegerextension to get storage)
• process spans and updates strategies in that storage (which then will be read by the extension's server)

All of this should require very minimal implementation, primarily just wiring up the existing components already implemented in app/collector/main

[yurishkuro]

The existing component (from jaeger v1) are in blue:

flowchart LR
    Receiver --> AdaptiveSamplingProcessor --> BatchProcessor --> Exporter
    Exporter -->|"(1) get storage"| JaegerStorageExension
    Exporter -->|"(2) write trace"| TraceStorage
    AdaptiveSamplingProcessor -->|"getStorage()"| StorageConfig

    OTEL_SDK[OTEL
             SDK]
    OTEL_SDK -->|"(1) GET /sampling"| HTTP_endpoint
    HTTP_endpoint -->|"(2) getStrategy()"| StrategiesProvider
    style HTTP_endpoint fill:blue,color:white

    subgraph Jaeger Collector
        Receiver
        BatchProcessor[Batch
                       Processor]
        Exporter        
        TraceStorage[(Trace
                      Storage)]
        AdaptiveSamplingProcessor[Adaptive
                                  Sampling
                                  Processor]
        AdaptiveSamplingProcessorV1[Adaptive
                                    Sampling
                                    Processor_v1]
        style AdaptiveSamplingProcessorV1 fill:blue,color:white
        AdaptiveSamplingProcessor -->|"[]*model.Span"| AdaptiveSamplingProcessorV1
        AdaptiveSamplingProcessorV1 ---|use| SamplingStorage

        subgraph JaegerStorageExension[Jaeger Storage Exension]
            Storage[[Storage
                     Config]]
        end
        subgraph RemoteSamplingExtension[Remote Sampling Extension]
            StrategiesProvider -->|"(3b) getStrategy()"| AdaptiveProvider
            StrategiesProvider -->|"(3a) getStrategy()"| FileProvider
            FileProvider --> FileConfig
            AdaptiveProvider --> StorageConfig
    
            HTTP_endpoint[HTTP
                          endpoint]
            StrategiesProvider[Strategies
                               Provider]
            FileProvider[File
                         Provider]
            AdaptiveProvider[Adaptive
                             Provider]
            style StrategiesProvider fill:blue,color:white
            style FileProvider fill:blue,color:white
            style AdaptiveProvider fill:blue,color:white
            subgraph Config
                FileConfig[[File Config]]
                StorageConfig[[Storage Config]]
            end
            StorageConfig --- SamplingStorage
            SamplingStorage[(Sampling
                             Storage)]
            style SamplingStorage fill:blue,color:white
        end
    end

[Pushkarm029]

i haven;t pushed latest changes yet.

[yurishkuro]

looks good, in the right direction

[yurishkuro]

Suggested change

	static:
	file: ./cmd/jaeger/sampling-strategies.json
	reload_interval: 1s
	file:
	path: ./cmd/jaeger/sampling-strategies.json
	reload_interval: 1s

[yurishkuro]

why do we need reload_interval: 1s? The current implementation reloads the file on changes, not on timer.

[Pushkarm029]

With its reload_interval: 1s, it does both; it reloads after every 1s + on file change.

You are right; reloading only when the file changes is better.

[Pushkarm029]

I think current implementation also supports reload_interval: see plugin/sampling/strategystore/static/strategy_store.go : L90

[yurishkuro]

File & adaptive sections do not make sense simultaneously, it's either one or another. Or am I missing something?

[Pushkarm029]

Yes, I already added a Conditional Check, which throws an error if both are provided.

Since these are production configs, I've included both options to make sure that the user is aware of their availability.

[Pushkarm029]

I have added a comment in the configuration to make it more clear.

[yurishkuro]

keeping them both means the config file is invalid. Config files are supposed to work without changes.

[yurishkuro]

Are you sure OTEL Collector does not already have a helper function for this? They do start various HTTP servers. I would check OTLP/HTTP receiver, z-pages extension.

[Pushkarm029]

I am not able to find any helper function.

In both OTLP/HTTP receiver, z-pages extension, a new http is initiated using http.NewServeMux().

[yurishkuro]

ok, well if OTEL does not have a helper function, we certainly do have it in jaeger-v1, so let's not duplicate (refactor into public helper if needed)

also, they do have helper that creates HTTP server from standard HTTP config, which presumably deals with TLS and things like that: https://github.com/open-telemetry/opentelemetry-collector/blob/c8f9563f1b12470cb74f2f98c226faaffbcb38ac/receiver/otlpreceiver/otlp.go#L148

[yurishkuro]

A lot of the parameters here are actually only needed for the adaptive sampling processor, which should run inside span processor, not inside the extension.

[Pushkarm029]

I tried doing it, but it makes the code more complicated. We call NewStrategyStore in the extension, which requires all options. However, in the adaptive sampling processor, we only need these options: AggregationBuckets and CalculationInterval to create a new aggregator that calculates probability and stores it.

But if we still need to pass this configuration to the processor, we must find a way to transport the configuration from processor to the remote sampling extension internally.

[Pushkarm029]

Currently, I cannot find any way to transport the configuration internally from the processor to the extension.

cc @james-ryans

[yurishkuro]

I was wrong, most params are indeed used by adaptive.Processor, which is confusing since it's very different in purpose from the new adaptivesampling processor that you added. I think it's actually a poor design choice in the old code, because what that Processor does is it periodically loads the throughput numbers, calculates new probabilities and stores them. Then the "strategy store" part, which is really needed for the HTTP endpoint, only reads the probabilities and constructs sampling strategies. We can see the evidence here - starting two goroutines at once is a smell, breaking single responsibility principle:

jaeger/plugin/sampling/strategystore/adaptive/processor.go

Lines 174 to 175 in 5ddcaa9

	p.runBackground(p.runCalculationLoop)
	p.runBackground(p.runUpdateProbabilitiesLoop)

Arguably a better design would've been for the Processor part to be co-located with the Aggregator (and run by adaptivesamping processor), and only retrieval/serving logic would've been in the remotesampling extension.

Think about it. Maybe we can do a bit of preemptive refactoring of the adaptive.Processor class:

• keep the part that runs in the background and updates storage with probabilities
• move the loadProbabilities/runUpdateProbabilitiesLoop part into strategy_store.go (as a separate class).

But one thing to be careful about is if we do this we want to ensure that these config options are cleanly split into two disjoined sets. Otherwise users would have to match the options across two OTEL components, something I would prefer to avoid. Right now they only share the storage name (if you follow my suggestion in another comment about maxBuckets).

[Pushkarm029]

Ok, I'm creating a new PR to refactor v1 sampling.

[yurishkuro]

starting http server is not part of file/adaptive choice, it's always there

[yurishkuro]

these if()s are getting very busy, I recommend extracting their content into helper functions

[yurishkuro]

let's add these parameters to configs where they must come from, it will make it easier to see how the configs are related

[yurishkuro]

You're only using ss to call ss.CreateSamplingStore(maxBuckets). Do you actually need remotesampling.GetSamplingStorageFactory, or perhaps only remotesampling.GetSamplingStorage (without maxBuckets argument)? This way you would not need to repeat maxBuckets config in the processor, only in the remotesampling extension

[yurishkuro]

Suggested change

// Close Aggregator

[yurishkuro]

you're missing a bunch of critical business logic

jaeger/cmd/collector/app/root_span_handler.go

Line 25 in 5ddcaa9

func handleRootSpan(aggregator strategystore.Aggregator, logger *zap.Logger) ProcessSpan {

I would suggest refactoring that logic into a helper method in aggregator.go like RecordThroughput(agg, span)

[yurishkuro]

the accurate name is aggregationInterval - it has to be a parameter of the processor/config

[yurishkuro]

ok, well if OTEL does not have a helper function, we certainly do have it in jaeger-v1, so let's not duplicate (refactor into public helper if needed)

also, they do have helper that creates HTTP server from standard HTTP config, which presumably deals with TLS and things like that: https://github.com/open-telemetry/opentelemetry-collector/blob/c8f9563f1b12470cb74f2f98c226faaffbcb38ac/receiver/otlpreceiver/otlp.go#L148

[yurishkuro]

I was wrong, most params are indeed used by adaptive.Processor, which is confusing since it's very different in purpose from the new adaptivesampling processor that you added. I think it's actually a poor design choice in the old code, because what that Processor does is it periodically loads the throughput numbers, calculates new probabilities and stores them. Then the "strategy store" part, which is really needed for the HTTP endpoint, only reads the probabilities and constructs sampling strategies. We can see the evidence here - starting two goroutines at once is a smell, breaking single responsibility principle:

jaeger/plugin/sampling/strategystore/adaptive/processor.go

Lines 174 to 175 in 5ddcaa9

	p.runBackground(p.runCalculationLoop)
	p.runBackground(p.runUpdateProbabilitiesLoop)

Arguably a better design would've been for the Processor part to be co-located with the Aggregator (and run by adaptivesamping processor), and only retrieval/serving logic would've been in the remotesampling extension.

Think about it. Maybe we can do a bit of preemptive refactoring of the adaptive.Processor class:

• keep the part that runs in the background and updates storage with probabilities
• move the loadProbabilities/runUpdateProbabilitiesLoop part into strategy_store.go (as a separate class).

But one thing to be careful about is if we do this we want to ensure that these config options are cleanly split into two disjoined sets. Otherwise users would have to match the options across two OTEL components, something I would prefer to avoid. Right now they only share the storage name (if you follow my suggestion in another comment about maxBuckets).

[yurishkuro]

temporary? It needs to be full blown server config, e.g. go.opentelemetry.io/collector/config/confighttp.ServerConfig