feat: add dependency processor using Apache Beam #6560

yunmaoQu · 2025-01-17T17:37:56Z

Which problem is this PR solving?

Resolves #5911

Description of the changes

add dependency processor using Apache Beam

How was this change tested?

e2e tests

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: npm run lint and npm run test

Signed-off-by: yunmaoQu <[email protected]>

yurishkuro · 2025-01-17T21:19:46Z

plugin/storage/memory/memory.go

-		perTenant:     make(map[string]*Tenant),
+		defaultConfig:      cfg,
+		perTenant:          make(map[string]*Tenant),
+		useNewDependencies: false, // 添加初始化


please use English in comments

cmd/jaeger/internal/processors/dependencyprocessor/aggregator.go

yurishkuro

where is it hooked up to anything?
what would be the e2e testing for this component?

yunmaoQu · 2025-01-18T02:52:23Z

where is it hooked up to anything?

what would be the e2e testing for this component?

@yurishkuro I have fixed it

Signed-off-by: yunmaoQu <[email protected]>

yurishkuro

@mahadzaryab1 interesting direction here

yurishkuro · 2025-01-19T15:48:55Z

cmd/jaeger/internal/processors/dependencyprocessor/config.go

+type Config struct {
+	AggregationInterval time.Duration `yaml:"aggregation_interval"`
+	InactivityTimeout   time.Duration `yaml:"inactivity_timeout"`
+	Store               *memory.Store `yaml:"-"`


Storage is not part of the config. The processor itself can retrieve storage from storage extension - see adaptive sampling processor for example.

cmd/jaeger/internal/processors/dependencyprocessor/config.go

yurishkuro · 2025-01-19T15:52:12Z

cmd/jaeger/internal/processors/dependencyprocessor/e2e_test.go

+	require.NoError(t, err)
+
+	// Wait for the processor to process the trace
+	time.Sleep(cfg.AggregationInterval + 100*time.Millisecond)


We don't use sleep in tests, use assert.Eventually

yurishkuro · 2025-01-19T15:55:12Z

cmd/jaeger/internal/processors/dependencyprocessor/config.go

+
+func DefaultConfig() Config {
+	return Config{
+		AggregationInterval: 5 * time.Second, // Default dependency aggregation interval: 5 seconds


This is way too frequent. We used to flush every 15min at Uber, since the topology does not change that often, but I can see a need for more frequent flushing if we want to capture movement of metrics.

Suggested change

AggregationInterval: 5 * time.Second, // Default dependency aggregation interval: 5 seconds

AggregationInterval: 10 * time.Minute,

The comment is redundant.

It would be nice to flush every minute to support investigating metrics movement, but it will create too much data. We may have to consider integrating with Prometheus instead, and only focus on capturing topology here, which definitely does not need 1min frequency.

cmd/jaeger/internal/processors/dependencyprocessor/factory.go

cmd/jaeger/internal/processors/dependencyprocessor/processor.go

cmd/jaeger/internal/processors/dependencyprocessor/e2e_test.go

Signed-off-by: yunmaoQu <[email protected]>

yunmaoQu · 2025-01-20T07:49:26Z

@yurishkuro Except this ,I update all based on your review.

yurishkuro · 2025-01-20T16:39:17Z

cmd/jaeger/internal/processors/dependencyprocessor/processor.go

+	config           *Config
+	aggregator       *dependencyAggregator // Define the aggregator below.
+	telset           component.TelemetrySettings
+	dependencyWriter *memory.Store


as I mentioned, you cannot have concrete store dependency here. The processor needs to work with any storage supported by Jaeger, as long as they implement WriteDependencies.

Example:

jaeger/cmd/jaeger/internal/extension/remotesampling/extension.go

Line 187 in 04b1ea4

f, err := jaegerstorage.GetStorageFactory(storageName, host)

yurishkuro · 2025-01-20T16:40:27Z

cmd/jaeger/internal/processors/dependencyprocessor/processor.go

+func (tp *dependencyProcessor) Shutdown(ctx context.Context) error {
+	close(tp.closeChan)
+	if tp.aggregator != nil {
+		if err := tp.aggregator.Close(); err != nil {


if aggregator has a Close() function why does it need to be passed closeChan?

yurishkuro · 2025-01-20T16:42:10Z

cmd/jaeger/internal/processors/dependencyprocessor/config.go

+type Config struct {
+	// AggregationInterval defines how often the processor aggregates dependencies.
+	// This controls the frequency of flushing dependency data to storage.
+	// Default dependency aggregation interval: 10 seconds


no need to mention default, it's defined elsewhere and we don't have to keep two places in sync.

yurishkuro · 2025-01-20T16:43:59Z

cmd/jaeger/internal/processors/dependencyprocessor/config.go

+	// AggregationInterval defines how often the processor aggregates dependencies.
+	// This controls the frequency of flushing dependency data to storage.


Suggested change

// AggregationInterval defines how often the processor aggregates dependencies.

// This controls the frequency of flushing dependency data to storage.

// AggregationInterval defines the length of aggregation window after

// which the accumulated dependencies are flushed into storage.

yurishkuro · 2025-01-20T16:44:27Z

cmd/jaeger/internal/processors/dependencyprocessor/e2e_test.go

+
+	cfg.AggregationInterval = 1 * time.Second


Suggested change

cfg.AggregationInterval = 1 * time.Second

cfg.AggregationInterval = 1 * time.Second

yurishkuro · 2025-01-20T16:45:50Z

cmd/jaeger/internal/processors/dependencyprocessor/config.go

+	// is considered complete and ready for dependency aggregation.
+	// Default trace completion timeout: 2 seconds of inactivity
+	InactivityTimeout time.Duration `yaml:"inactivity_timeout"`
+}


please add Validate method and use valid: notations in the field tags.

yurishkuro · 2025-01-20T16:55:19Z

cmd/jaeger/internal/processors/dependencyprocessor/aggregator.go

+	"github.com/jaegertracing/jaeger/plugin/storage/memory"
+)
+
+type dependencyAggregator struct {


there is a lot of code here. How can it be unit tested with beam?

yurishkuro · 2025-01-20T16:56:01Z

cmd/jaeger/internal/processors/dependencyprocessor/factory.go

+		AggregationInterval: 10 * time.Second,
+		InactivityTimeout:   2 * time.Second,


Suggested change

AggregationInterval: 10 * time.Second,

InactivityTimeout: 2 * time.Second,

AggregationInterval: 10 * time.Minute,

InactivityTimeout: 2 * time.Minute,

yurishkuro · 2025-01-20T16:57:46Z

cmd/jaeger/internal/processors/dependencyprocessor/processor.go

+}
+
+func (dp *dependencyProcessor) ConsumeTraces(ctx context.Context, td ptrace.Traces) error {
+	batches := v1adapter.ProtoFromTraces(td)


why do we need to fall back to the old Jaeger model? This is new code, let's keep it in the ptrace domain.

yurishkuro · 2025-01-20T17:04:18Z

cmd/jaeger/internal/processors/dependencyprocessor/aggregator.go

+}
+
+func newDependencyAggregator(cfg Config, telset component.TelemetrySettings, dependencyWriter *memory.Store) *dependencyAggregator {
+	beam.Init()


can this be called more than once?

yurishkuro · 2025-01-20T17:15:08Z

cmd/jaeger/internal/processors/dependencyprocessor/e2e_test.go

+	"github.com/jaegertracing/jaeger/plugin/storage/memory"
+)
+
+func TestDependencyProcessorEndToEnd(t *testing.T) {


note: this is a unit test, not an e2e test. By e2e I mean running the new processor inside a real Jaeger binary and validating the correct processing through the API calls. See cmd/jaeger/internal/integration/.

thanks a lot. I will fix it

yunmaoQu requested a review from a team as a code owner January 17, 2025 17:37

yunmaoQu requested a review from joe-elliott January 17, 2025 17:37

feat: add dependency processor using Apache Beam

60fb334

Signed-off-by: yunmaoQu <[email protected]>

yunmaoQu force-pushed the add-dependency-processor branch from af5f794 to 60fb334 Compare January 17, 2025 17:42

yurishkuro reviewed Jan 17, 2025

View reviewed changes

cmd/jaeger/internal/processors/dependencyprocessor/aggregator.go Show resolved Hide resolved

yurishkuro reviewed Jan 17, 2025

View reviewed changes

fix: address review comments

5ccf68d

Signed-off-by: yunmaoQu <[email protected]>

yurishkuro reviewed Jan 19, 2025

View reviewed changes

update

05dc78f

Signed-off-by: yunmaoQu <[email protected]>

yurishkuro reviewed Jan 20, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add dependency processor using Apache Beam #6560

feat: add dependency processor using Apache Beam #6560

yunmaoQu commented Jan 17, 2025

yurishkuro Jan 17, 2025

yunmaoQu Jan 18, 2025

yurishkuro left a comment

yunmaoQu commented Jan 18, 2025 •

edited

Loading

yurishkuro left a comment

yurishkuro Jan 19, 2025

yurishkuro Jan 19, 2025

yurishkuro Jan 19, 2025

yurishkuro Jan 19, 2025

yunmaoQu commented Jan 20, 2025 •

edited

Loading

yurishkuro Jan 20, 2025

yurishkuro Jan 20, 2025

yurishkuro Jan 20, 2025

yurishkuro Jan 20, 2025

yurishkuro Jan 20, 2025

yurishkuro Jan 20, 2025

yunmaoQu Jan 21, 2025

yurishkuro Jan 20, 2025

yunmaoQu Jan 21, 2025

yurishkuro Jan 20, 2025

yurishkuro Jan 20, 2025

yurishkuro Jan 20, 2025

yurishkuro Jan 20, 2025

yunmaoQu Jan 21, 2025

	AggregationInterval: 5 * time.Second, // Default dependency aggregation interval: 5 seconds
	AggregationInterval: 10 * time.Minute,

		// AggregationInterval defines how often the processor aggregates dependencies.
		// This controls the frequency of flushing dependency data to storage.


	cfg.AggregationInterval = 1 * time.Second
	cfg.AggregationInterval = 1 * time.Second

		AggregationInterval: 10 * time.Second,
		InactivityTimeout: 2 * time.Second,

feat: add dependency processor using Apache Beam #6560

Are you sure you want to change the base?

feat: add dependency processor using Apache Beam #6560

Conversation

yunmaoQu commented Jan 17, 2025

Which problem is this PR solving?

Description of the changes

How was this change tested?

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yurishkuro left a comment

Choose a reason for hiding this comment

yunmaoQu commented Jan 18, 2025 • edited Loading

yurishkuro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yunmaoQu commented Jan 20, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yunmaoQu commented Jan 18, 2025 •

edited

Loading

yunmaoQu commented Jan 20, 2025 •

edited

Loading