Add flag to enable span size metrics reporting #3782

ymtdzzz · 2022-06-26T23:56:16Z

Which problem is this PR solving?

Resolves #2126

Short description of the changes

add --collector.enable-span-size-metrics that enables processed span metrics even if --collector.queue-size-memory is disabled

Details

--collector.enable-span-size-metrics is enabled but --collector.queue-size-memory is disabled

apiVersion: jaegertracing.io/v1
kind: Jaeger
metadata:
  name: jaeger
spec:
  strategy: AllInOne
  imagePullPolicy: Never
  allInOne:
    image: jaegeraio:wip
    options:
      collector:
        enable-span-size-metrics: true

--collector.enable-processed-span-metrics is disabled but --collector.queue-size-memory is enabled (no change)

apiVersion: jaegertracing.io/v1
kind: Jaeger
metadata:
  name: jaeger
spec:
  strategy: AllInOne
  imagePullPolicy: Never
  allInOne:
    image: jaegeraio:wip
    options:
      collector:
        queue-size-memory: 10

Signed-off-by: Yosuke Matsuda <[email protected]>

ymtdzzz · 2022-06-27T00:21:21Z

cmd/collector/app/span_processor.go

 		options.logger.Info("Dynamically adjusting the queue size at runtime.",
 			zap.Uint("memory-mib", options.dynQueueSizeMemory/1024/1024),
 			zap.Uint("queue-size-warmup", options.dynQueueSizeWarmup))
+	}
+	if options.dynQueueSizeMemory > 0 || options.processedSpanMetricsEnabled {
+		// add to processSpanFuncs
 		processSpanFuncs = append(processSpanFuncs, sp.countSpan)


As mentioned in #2126 (comment) , sp.spansProcessed.Inc() could be independent but I didn't change this for simplicity.

cmd/collector/app/span_processor.go

yurishkuro · 2022-06-28T00:42:46Z

cmd/collector/app/flags/flags.go

+	flagNumWorkers                  = "collector.num-workers"
+	flagQueueSize                   = "collector.queue-size"
+	flagCollectorTags               = "collector.tags"
+	flagProcessedSpanMetricsEnabled = "collector.enable-processed-span-metrics"


Suggested change

flagProcessedSpanMetricsEnabled = "collector.enable-processed-span-metrics"

flagProcessedSpanMetricsEnabled = "collector.enable-span-size-metrics"

yurishkuro · 2022-06-28T00:43:47Z

cmd/collector/app/flags/flags.go

@@ -163,6 +166,7 @@ func AddFlags(flags *flag.FlagSet) {
 	flags.Int(flagQueueSize, DefaultQueueSize, "The queue size of the collector")
 	flags.Uint(flagDynQueueSizeMemory, 0, "(experimental) The max memory size in MiB to use for the dynamic queue.")
 	flags.String(flagCollectorTags, "", "One or more tags to be added to the Process tags of all spans passing through this collector. Ex: key1=value1,key2=${envVar:defaultValue}")
+	flags.Bool(flagProcessedSpanMetricsEnabled, false, "Enables processed span metrics.")


Suggested change

flags.Bool(flagProcessedSpanMetricsEnabled, false, "Enables processed span metrics.")

flags.Bool(flagProcessedSpanMetricsEnabled, false, "Enables metrics based on processed span size, which are more expensive to calculate.")

yurishkuro · 2022-06-28T00:44:10Z

cmd/collector/app/flags/flags.go

@@ -124,6 +125,8 @@ type CollectorOptions struct {
 	}
 	// CollectorTags is the string representing collector tags to append to each and every span
 	CollectorTags map[string]string
+	// ProcessedSpanMetricsEnabled determines whether to enable processed span metrics
+	ProcessedSpanMetricsEnabled bool


Suggested change

ProcessedSpanMetricsEnabled bool

SpanSizeMetricsEnabled bool

yurishkuro · 2022-06-28T05:47:26Z

cmd/collector/app/flags/flags.go

+	flagNumWorkers                  = "collector.num-workers"
+	flagQueueSize                   = "collector.queue-size"
+	flagCollectorTags               = "collector.tags"
+	flagProcessedSpanMetricsEnabled = "collector.enable-span-size-metrics"


Suggested change

flagProcessedSpanMetricsEnabled = "collector.enable-span-size-metrics"

flagSpanSizeMetricsEnabled = "collector.enable-span-size-metrics"

let's keep consistent naming

oh ok, I will check it all again

yurishkuro · 2022-06-28T05:48:35Z

cmd/collector/app/options.go

+	reportBusy                  bool
+	extraFormatTypes            []processor.SpanFormat
+	collectorTags               map[string]string
+	processedSpanMetricsEnabled bool


please keep naming consistent

yurishkuro · 2022-06-28T05:48:41Z

cmd/collector/app/options.go

@@ -156,6 +157,13 @@ func (options) CollectorTags(extraTags map[string]string) Option {
 	}
 }

+// ProcessedSpanMetricsEnabled creates an Option that initializes the processedSpanMetrics boolean
+func (options) ProcessedSpanMetricsEnabled(processedSpanMetrics bool) Option {


please keep naming consistent

Signed-off-by: Yosuke Matsuda <[email protected]>

yurishkuro · 2022-06-28T16:01:16Z

cmd/collector/app/span_processor_test.go

+			if tt.expectedUpdateGauge {
+				assert.Fail(t, "gauge hasn't been updated within a reasonable amount of time")
+			}
+			assert.NoError(t, p.Close())


this won't be executed if you return in L460. I suggest using defer to close p.

yurishkuro · 2022-06-28T16:02:49Z

cmd/collector/app/span_processor_test.go

+		expectedUpdateGauge   bool
+	}{
+		{
+			name:                  "enable-dyn-queue-size-enable-metrics",


Suggested change

name: "enable-dyn-queue-size-enable-metrics",

name: "enable dyn-queue-size, enable metrics",

nit: remove dashes for better readability, separate with commas

yurishkuro · 2022-06-28T16:04:28Z

cmd/collector/app/span_processor_test.go

+			p.processSpan(&model.Span{}, "")
+			assert.NotEqual(t, uint64(0), p.bytesProcessed)
+
+			for i := 0; i < 15; i++ {


Suggested change

for i := 0; i < 15; i++ {

for i := 0; i < 10000; i++ {

doesn't affect test speed in most cases, but makes it more resilient in case of VM freezes

Signed-off-by: Yosuke Matsuda <[email protected]>

yurishkuro

thanks!

codecov · 2022-06-29T01:16:14Z

Codecov Report

Merging #3782 (32c8209) into main (9b4bbf4) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #3782      +/-   ##
==========================================
+ Coverage   97.57%   97.59%   +0.01%     
==========================================
  Files         288      288              
  Lines       16765    16774       +9     
==========================================
+ Hits        16359    16370      +11     
+ Misses        321      319       -2     
  Partials       85       85

Impacted Files	Coverage Δ
cmd/collector/app/flags/flags.go	`96.80% <100.00%> (+0.06%)`	⬆️
cmd/collector/app/options.go	`100.00% <100.00%> (ø)`
cmd/collector/app/span_handler_builder.go	`100.00% <100.00%> (ø)`
cmd/collector/app/span_processor.go	`100.00% <100.00%> (ø)`
pkg/config/tlscfg/cert_watcher.go	`94.68% <0.00%> (+2.12%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9b4bbf4...32c8209. Read the comment docs.

* Add parameter to enable processed span metrics Signed-off-by: Yosuke Matsuda <[email protected]> * Rename some fields `processedSpanMetrics` to `spanSizeMetrics` Signed-off-by: Yosuke Matsuda <[email protected]> * Improve test reliability and readability Signed-off-by: Yosuke Matsuda <[email protected]> Signed-off-by: Albert Teoh <[email protected]>

Add parameter to enable processed span metrics

1b22e9e

Signed-off-by: Yosuke Matsuda <[email protected]>

ymtdzzz marked this pull request as ready for review June 27, 2022 00:16

ymtdzzz requested a review from a team as a code owner June 27, 2022 00:16

ymtdzzz requested a review from pavolloffay June 27, 2022 00:16

ymtdzzz commented Jun 27, 2022

View reviewed changes

yurishkuro reviewed Jun 27, 2022

View reviewed changes

cmd/collector/app/span_processor.go Show resolved Hide resolved

yurishkuro reviewed Jun 28, 2022

View reviewed changes

Rename some fields processedSpanMetrics to spanSizeMetrics

c4282bf

Signed-off-by: Yosuke Matsuda <[email protected]>

ymtdzzz force-pushed the feature/collector/add-span-metrics-param branch from 61b230b to c4282bf Compare June 28, 2022 10:13

yurishkuro reviewed Jun 28, 2022

View reviewed changes

Improve test reliability and readability

32c8209

Signed-off-by: Yosuke Matsuda <[email protected]>

yurishkuro approved these changes Jun 29, 2022

View reviewed changes

yurishkuro changed the title ~~Add parameter to enable processed span metrics~~ Add flag to enable span size metrics reporting Jun 29, 2022

yurishkuro enabled auto-merge (squash) June 29, 2022 01:10

yurishkuro merged commit 10af8ec into jaegertracing:main Jun 29, 2022

ymtdzzz deleted the feature/collector/add-span-metrics-param branch June 29, 2022 01:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add flag to enable span size metrics reporting #3782

Add flag to enable span size metrics reporting #3782

ymtdzzz commented Jun 26, 2022 •

edited

Loading

ymtdzzz Jun 27, 2022

yurishkuro Jun 28, 2022

yurishkuro Jun 28, 2022

yurishkuro Jun 28, 2022

yurishkuro Jun 28, 2022 •

edited

Loading

ymtdzzz Jun 28, 2022

yurishkuro Jun 28, 2022

yurishkuro Jun 28, 2022

yurishkuro Jun 28, 2022

yurishkuro Jun 28, 2022

yurishkuro Jun 28, 2022

yurishkuro left a comment

codecov bot commented Jun 29, 2022

	flagProcessedSpanMetricsEnabled = "collector.enable-processed-span-metrics"
	flagProcessedSpanMetricsEnabled = "collector.enable-span-size-metrics"

	flags.Bool(flagProcessedSpanMetricsEnabled, false, "Enables processed span metrics.")
	flags.Bool(flagProcessedSpanMetricsEnabled, false, "Enables metrics based on processed span size, which are more expensive to calculate.")

	flagProcessedSpanMetricsEnabled = "collector.enable-span-size-metrics"
	flagSpanSizeMetricsEnabled = "collector.enable-span-size-metrics"

	name: "enable-dyn-queue-size-enable-metrics",
	name: "enable dyn-queue-size, enable metrics",

Add flag to enable span size metrics reporting #3782

Add flag to enable span size metrics reporting #3782

Conversation

ymtdzzz commented Jun 26, 2022 • edited Loading

Which problem is this PR solving?

Short description of the changes

Details

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

yurishkuro Jun 28, 2022 • 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

yurishkuro left a comment

Choose a reason for hiding this comment

codecov bot commented Jun 29, 2022

Codecov Report

ymtdzzz commented Jun 26, 2022 •

edited

Loading

yurishkuro Jun 28, 2022 •

edited

Loading