COSI-61 disable trace export if endpoint not specified(default) and test new default behaviour in e2e feature tests (stdout export remains same) #89

anurag4DSB · 2025-01-08T11:46:12Z

OpenTelemetry Configuration Updates
• Default Endpoint: Modified the default OpenTelemetry endpoint to be an empty string ("") to avoid unintended errors if no trace collector endpoint is provided.
• Tracing Logic: Updated logic to disable tracing when both otel_stdout and otel_endpoint are unset.
• Service Name: Added driverOtelServiceName to the initialization process log for better trace span identification.
Helm Chart Enhancements
• Improved Comments: Enhanced the values.yaml file with detailed comments explaining the usage of otel_stdout and otel_endpoint.
• Default Values: Updated the default values to ensure trace collection is explicitly configured by the user, avoiding unintentional trace exports.
Script Adjustments for E2E Testing
• Disabled tracing in e2e feature tests to verify the system’s default behavior without tracing enabled.

codecov · 2025-01-08T11:50:04Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.47%. Comparing base (be74859) to head (2d4b483).
Report is 4 commits behind head on main.

Additional details and impacted files

Components	Coverage Δ
🏠 Main Package	`∅ <ø> (∅)`
🚗 Driver Package	`92.22% <ø> (ø)`
📡 gRPC Factory Package	`82.35% <ø> (ø)`
🔐 IAM Client Package	`98.17% <ø> (ø)`
🌐 S3 Client Package	`94.33% <ø> (ø)`
🔧 Util Package	`100.00% <ø> (ø)`
🔖 Constants Package	`∅ <ø> (∅)`

@@           Coverage Diff           @@
##             main      #89   +/-   ##
=======================================
  Coverage   92.47%   92.47%           
=======================================
  Files          11       11           
  Lines         771      771           
=======================================
  Hits          713      713           
  Misses         49       49           
  Partials        9        9

anurag4DSB · 2025-01-08T12:07:47Z

helm/scality-cosi-driver/values.yaml

+  # Configure tracing for the application.
+
+  # If both `otel_stdout` and `otel_endpoint` are set, `otel_stdout` takes precedence.
+  # Set `otel_stdout: false` and `otel_endpoint: ""` to disable tracing entirely.
+
+  # The endpoint of the trace collector to which traces will be sent.
+  # Use an empty string ("") to disable endpoint-based trace collection.
+  # Use the format "http://<host>:<port>/v1/trace" to specify the collector endpoint.
+  otel_endpoint: ""
+
+  # Enable stdout tracing by setting this to true. When enabled, traces will be printed
+  # to the console instead of being sent to the collector endpoint.
  otel_stdout: false
-  # Trace span service name
+
+  # The name of the service to appear in trace spans, used for identification
+  # in observability tools.
  otel_service_name: "cosi.scality.com"


These are defaults.

In the CI we do have tracing enabled as we enable it using --set traces.otel_stdout=true

helm install scality-cosi-driver ./helm/scality-cosi-driver \ --namespace scality-object-storage \ --create-namespace \ --set image.tag=latest \ --set traces.otel_stdout=true

fredmnl · 2025-01-08T17:38:26Z

cmd/scality-cosi-driver/cmd.go

@@ -98,13 +99,17 @@ func initOpenTelemetry(ctx context.Context) (*sdktrace.TracerProvider, error) {
 			return nil, fmt.Errorf("failed to initialize stdout exporter: %w", err)
 		}
 		klog.Info("OpenTelemetry tracing enabled with stdout exporter")
-	} else {
-		// Configure OTLP exporter
+	} else if *driverOtelEndpoint != "" {


Is this guaranteed to be non-nil?

Yes, I think *driverOtelEndpoint is guaranteed to be non-nil because it is defined as a pointer to a string variable initialized via the flag.String function.

The flag.String function initializes driverOtelEndpoint with a valid pointer to the default value (defaultOtelEndpoint), which is an empty string (""). This means that driverOtelEndpoint will never be nil during program execution, as long as flag.Parse() is called before accessing the variable.

Are you seeing something I do not see yet?

No you're absolutely right. It's just so strange that flag.String returns a *string even though it never returns a nil value. I guess it's nil until the call to the flag package is done.

LGTM

So, before flag.Parse(): Technically, the flag.String call initializes the pointer immediately. Thus, even before flag.Parse(), the pointer itself is not nil. However, the value it points to will not reflect any command-line overrides until flag.Parse() is invoked.

I have that strange feeling as well, for me it came from the expectation pointers to potentially be nil. But in the case of the flag package, the pointers are always initialized because flag.String (and its siblings) guarantee that.

var example = flag.String("example", "default", "an example flag") fmt.Println(*example) // Safe: Prints "default" even before flag.Parse()

fredmnl · 2025-01-09T10:32:11Z

cmd/scality-cosi-driver/cmd.go

@@ -98,13 +99,17 @@ func initOpenTelemetry(ctx context.Context) (*sdktrace.TracerProvider, error) {
 			return nil, fmt.Errorf("failed to initialize stdout exporter: %w", err)
 		}
 		klog.Info("OpenTelemetry tracing enabled with stdout exporter")
-	} else {
-		// Configure OTLP exporter
+	} else if *driverOtelEndpoint != "" {


No you're absolutely right. It's just so strange that flag.String returns a *string even though it never returns a nil value. I guess it's nil until the call to the flag package is done.

LGTM

- Default functionality is to not export traces - Updated e2e tests for help to verify the same

anurag4DSB force-pushed the improvement/COSI-61-disable-trace-export-if-endpoint-not-specified branch from 95fe631 to 06f9ffa Compare January 8, 2025 12:06

anurag4DSB commented Jan 8, 2025

View reviewed changes

anurag4DSB changed the title ~~COSI-61 disable trace export if endpoint not specified and capture logs for helm validation~~ COSI-61 disable trace export if endpoint not specified and test new default behaviour Jan 8, 2025

anurag4DSB changed the title ~~COSI-61 disable trace export if endpoint not specified and test new default behaviour~~ COSI-61 disable trace export if endpoint not specified(default) and test new default behaviour Jan 8, 2025

anurag4DSB changed the title ~~COSI-61 disable trace export if endpoint not specified(default) and test new default behaviour~~ COSI-61 disable trace export if endpoint not specified(default) and test new default behaviour in e2e feature tests (stdout export remains same) Jan 8, 2025

anurag4DSB marked this pull request as ready for review January 8, 2025 12:13

BourgoisMickael approved these changes Jan 8, 2025

View reviewed changes

fredmnl reviewed Jan 8, 2025

View reviewed changes

anurag4DSB requested a review from fredmnl January 8, 2025 17:42

fredmnl approved these changes Jan 9, 2025

View reviewed changes

anurag4DSB force-pushed the feature/COSI-61-enable-otel-traces-for-cosi-driver branch 3 times, most recently from 2f8bbe8 to be74859 Compare January 9, 2025 10:50

Disable trace export if endpoint not specified

2d4b483

- Default functionality is to not export traces - Updated e2e tests for help to verify the same

anurag4DSB force-pushed the improvement/COSI-61-disable-trace-export-if-endpoint-not-specified branch from 06f9ffa to 2d4b483 Compare January 9, 2025 10:54

Base automatically changed from feature/COSI-61-enable-otel-traces-for-cosi-driver to main January 9, 2025 10:57

anurag4DSB merged commit f028105 into main Jan 9, 2025
6 checks passed

anurag4DSB deleted the improvement/COSI-61-disable-trace-export-if-endpoint-not-specified branch January 9, 2025 11:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

COSI-61 disable trace export if endpoint not specified(default) and test new default behaviour in e2e feature tests (stdout export remains same) #89

COSI-61 disable trace export if endpoint not specified(default) and test new default behaviour in e2e feature tests (stdout export remains same) #89

anurag4DSB commented Jan 8, 2025 •

edited

Loading

codecov bot commented Jan 8, 2025 •

edited

Loading

anurag4DSB Jan 8, 2025

fredmnl Jan 8, 2025

anurag4DSB Jan 8, 2025 •

edited

Loading

fredmnl Jan 9, 2025

anurag4DSB Jan 9, 2025

anurag4DSB Jan 9, 2025 •

edited

Loading

fredmnl Jan 9, 2025

COSI-61 disable trace export if endpoint not specified(default) and test new default behaviour in e2e feature tests (stdout export remains same) #89

COSI-61 disable trace export if endpoint not specified(default) and test new default behaviour in e2e feature tests (stdout export remains same) #89

Conversation

anurag4DSB commented Jan 8, 2025 • edited Loading

codecov bot commented Jan 8, 2025 • edited Loading

Codecov Report

anurag4DSB Jan 8, 2025

Choose a reason for hiding this comment

fredmnl Jan 8, 2025

Choose a reason for hiding this comment

anurag4DSB Jan 8, 2025 • edited Loading

Choose a reason for hiding this comment

fredmnl Jan 9, 2025

Choose a reason for hiding this comment

anurag4DSB Jan 9, 2025

Choose a reason for hiding this comment

anurag4DSB Jan 9, 2025 • edited Loading

Choose a reason for hiding this comment

fredmnl Jan 9, 2025

Choose a reason for hiding this comment

anurag4DSB commented Jan 8, 2025 •

edited

Loading

codecov bot commented Jan 8, 2025 •

edited

Loading

anurag4DSB Jan 8, 2025 •

edited

Loading

anurag4DSB Jan 9, 2025 •

edited

Loading