perf(storage): allow dynamic enable/disable minitrace for tiered cache #17433

MrCroxx · 2024-06-25T03:40:37Z

Signed-off-by: MrCroxx [email protected]I hereby agree to the terms of the RisingWave Labs, Inc. Contributor License Agreement.

What's changed and what's your intention?

Introduce system parameters to dynamically enable/disable and configure minitrace for tiered cache.

dev=> SHOW PARAMETERS;
                  Name                  |       Value        |                       Description                       | Mutable
----------------------------------------+--------------------+---------------------------------------------------------+---------
 # ... ...
  tracing_threshold_tiered_cache_read_ms         | 1000               | Threshold to record tiered cache read with minitrace.   | t
  tracing_threshold_tiered_cache_write_ms        | 1000               | Threshold to record tiered cache write with minitrace.  | t

To enable tracing, set opentelemetry endpoint via env RW_TRACING_ENDPOINT and set system parameter enable_tracing as true.

Checklist

I have written necessary rustdoc comments
I have added necessary unit tests and integration tests
I have added test labels as necessary. See details.
I have added fuzzing tests or opened an issue to track them. (Optional, recommended for new SQL features Sqlsmith: Sql feature generation #7934).
My PR contains breaking changes. (If it deprecates some features, please create a tracking issue to remove them in the future).
All checks passed in ./risedev check (or alias, ./risedev c)
My PR changes performance-critical code. (Please run macro/micro-benchmarks and show the results.)

My PR contains critical fixes that are necessary to be merged into the latest release. (Please check out the details)

Documentation

My PR needs documentation updates. (Please use the Release note section below to summarize the impact on users)

Release note

If this PR includes changes that directly affect users or other significant modifications relevant to the community, kindly draft a release note to provide a concise summary of these changes. Please prioritize highlighting the impact these changes will have on users.

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

MrCroxx · 2024-06-25T07:28:39Z

Discussed offline with @tabVersion and @BugenZhao . The jaeger exporter will be replaced with OpenTelemetry exporter and reuse the env of the distributed tracing.

yezizp2012

LGTM for the system parameter part.

src/common/src/system_param/mod.rs

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

hzxa21

LGTM

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

BugenZhao

and set system parameter enable_tracing as true.

This doesn't seem to be implemented? Shall we also handle the change on system parameter enable_tracing for minitrace?

risingwave/src/common/src/system_param/common.rs

Lines 35 to 38 in a931b1e

    
           // Toggle the distributed tracing layer. 
        
           if let Some(enabled) = diff.enable_tracing { 
        
               toggle_otel_layer(enabled); 
        
           }

BugenZhao · 2024-07-03T02:40:22Z

proto/meta.proto

+  optional uint32 tracing_threshold_tiered_cache_read_ms = 18;
+  optional uint32 tracing_threshold_tiered_cache_write_ms = 19;


Personally I would suggest making them configuration entries (like under [storage.cache.developer]) instead of system parameters, since it looks to be less user-facing and does not require frequent online changes. 😕

since it looks to be less user-facing and does not require frequent online changes

+1. But the thing is, tail-based tracing is supposed to be enabled for only a few seconds to capture abnormal spans in the current state. Using a config file requires a restart, which may destroy the scene.

Can we simply use default values that are small enough and rely only on enable_tracing to toggle it?

Maybe enable/disable with RPC is better?

Can you elaborate more?

Like we do now to capture await tree and other profiling data, add a RPC function to control the tracing configurations.

risingwave/proto/monitor_service.proto

Lines 68 to 75 in 3c102d5

service MonitorService {

rpc StackTrace(StackTraceRequest) returns (StackTraceResponse);

rpc Profiling(ProfilingRequest) returns (ProfilingResponse);

rpc HeapProfiling(HeapProfilingRequest) returns (HeapProfilingResponse);

rpc ListHeapProfiling(ListHeapProfilingRequest) returns (ListHeapProfilingResponse);

rpc AnalyzeHeap(AnalyzeHeapRequest) returns (AnalyzeHeapResponse);

rpc GetBackPressure(GetBackPressureRequest) returns (GetBackPressureResponse);

}

BugenZhao · 2024-07-03T02:45:07Z

src/utils/runtime/src/logger.rs

-                .unwrap()
+                .unwrap();
+
+            let exporter = opentelemetry_otlp::new_exporter()


What's the difference between this exporter with the one in L449? Can we reuse them?

The interface requires a builder. 🥹

BugenZhao · 2024-07-03T02:45:20Z

src/utils/runtime/src/logger.rs

+        let reporter = OpenTelemetryReporter::new(
+            exporter,
+            SpanKind::Server,
+            Cow::Owned(Resource::new([KeyValue::new(


The resource can be reused as well.

The service name should be different from the distributed tracing.

Interesting. Is it because there is currently no way to maintain the parent relationship between tracing spans and minitrace spans? So we'd rather make them not interleaving with each other by assigning a different service name.

Emmm, nop. It is because I think it is not a good idea to assign the distributed tracing and lib-level tracing in the same service name. 👀 Separating them can give a clearer view.

MrCroxx · 2024-07-04T05:59:52Z

Discussed with @BugenZhao offline, I'll switch to rpc with risectl.

MrCroxx added 2 commits June 24, 2024 18:22

refactor(compute): add tracing related args

2ee7576

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

refactor(meta): add system parameters for minitrace

53eab7f

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

MrCroxx self-assigned this Jun 25, 2024

github-actions bot added the type/perf label Jun 25, 2024

MrCroxx added 2 commits June 25, 2024 14:10

feat: update foyer trace config on system parameter updates

f0eb1a7

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

chore: remove unused code

0070bd0

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

MrCroxx marked this pull request as ready for review June 25, 2024 06:29

MrCroxx requested a review from a team as a code owner June 25, 2024 06:29

MrCroxx requested review from hzxa21 and yezizp2012 June 25, 2024 06:29

MrCroxx added 2 commits June 25, 2024 14:49

feat: run background tiered cache reconfigurer

9f71a7d

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

Merge remote-tracking branch 'origin/main' into xx/tracing

8605806

yezizp2012 approved these changes Jun 25, 2024

View reviewed changes

BugenZhao self-requested a review June 25, 2024 09:21

BugenZhao reviewed Jun 25, 2024

View reviewed changes

src/common/src/system_param/mod.rs Outdated Show resolved Hide resolved

MrCroxx added 8 commits June 25, 2024 17:48

chore: upgrade foyer

ffb6c6c

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

chore: upgrade foyer

22ea63a

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

refactor: reuse opentelemetry endpoint for minitrace

272bb0f

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

fix: opentelemetry export build within runtime env

b5dc877

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

refactor: remove minitrace sp and rename other sps

dd115b4

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

Merge remote-tracking branch 'origin/main' into xx/tracing

a040bba

fix: fix cargo lock

2c479a3

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

chore: make dylint happy

cf3d115

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

hzxa21 approved these changes Jul 1, 2024

View reviewed changes

Merge remote-tracking branch 'origin/main' into xx/tracing

3c43b65

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

BugenZhao reviewed Jul 3, 2024

View reviewed changes

MrCroxx closed this Jul 4, 2024

MrCroxx deleted the xx/tracing branch July 4, 2024 06:00

MrCroxx mentioned this pull request Jul 4, 2024

perf(storage): allow dynamic enable/disable minitrace for tiered cache #17570

Merged

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

perf(storage): allow dynamic enable/disable minitrace for tiered cache #17433

perf(storage): allow dynamic enable/disable minitrace for tiered cache #17433

MrCroxx commented Jun 25, 2024 •

edited

Loading

MrCroxx commented Jun 25, 2024

yezizp2012 left a comment

hzxa21 left a comment

BugenZhao left a comment •

edited

Loading

BugenZhao Jul 3, 2024

MrCroxx Jul 3, 2024

BugenZhao Jul 3, 2024

MrCroxx Jul 3, 2024

BugenZhao Jul 3, 2024

MrCroxx Jul 4, 2024

BugenZhao Jul 3, 2024

MrCroxx Jul 4, 2024

BugenZhao Jul 3, 2024

MrCroxx Jul 3, 2024

BugenZhao Jul 3, 2024

MrCroxx Jul 4, 2024

MrCroxx commented Jul 4, 2024

	// Toggle the distributed tracing layer.
	if let Some(enabled) = diff.enable_tracing {
	toggle_otel_layer(enabled);
	}

		optional uint32 tracing_threshold_tiered_cache_read_ms = 18;
		optional uint32 tracing_threshold_tiered_cache_write_ms = 19;

	service MonitorService {
	rpc StackTrace(StackTraceRequest) returns (StackTraceResponse);
	rpc Profiling(ProfilingRequest) returns (ProfilingResponse);
	rpc HeapProfiling(HeapProfilingRequest) returns (HeapProfilingResponse);
	rpc ListHeapProfiling(ListHeapProfilingRequest) returns (ListHeapProfilingResponse);
	rpc AnalyzeHeap(AnalyzeHeapRequest) returns (AnalyzeHeapResponse);
	rpc GetBackPressure(GetBackPressureRequest) returns (GetBackPressureResponse);
	}

perf(storage): allow dynamic enable/disable minitrace for tiered cache #17433

perf(storage): allow dynamic enable/disable minitrace for tiered cache #17433

Conversation

MrCroxx commented Jun 25, 2024 • edited Loading

What's changed and what's your intention?

Checklist

Documentation

Release note

MrCroxx commented Jun 25, 2024

yezizp2012 left a comment

Choose a reason for hiding this comment

hzxa21 left a comment

Choose a reason for hiding this comment

BugenZhao left a comment • 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

MrCroxx commented Jul 4, 2024

MrCroxx commented Jun 25, 2024 •

edited

Loading

BugenZhao left a comment •

edited

Loading