[DOC] Adding missing `metric-generator` keys #3643

WesselAtWork · 2024-05-02T11:45:47Z

What this PR does:

Adds missing keys to the metrics-generator

Which issue(s) this PR fixes:
Fixes #3642

Sources/References

`override_ring_key`

tempo/modules/generator/config.go

Line 49 in 251bf5a

cfg.QueryTimeout = 30 * time.Second

`override_ring_key`

tempo/modules/generator/config.go

Line 50 in 251bf5a

cfg.OverrideRingKey = generatorRingKey

tempo/modules/generator/config.go

Lines 18 to 19 in 251bf5a

    
           // generatorRingKey is the default key under which we store the metric-generator's ring in the KVStore. 
        
           generatorRingKey = "metrics-generator"

`processor.service_graphs.peer_attributes`

tempo/modules/generator/processor/servicegraphs/config.go

Lines 54 to 57 in 251bf5a

    
           peerAttr := make([]string, 0, len(defaultPeerAttributes)) 
        
           for _, attr := range defaultPeerAttributes { 
        
           	peerAttr = append(peerAttr, string(attr)) 
        
           }

tempo/modules/generator/processor/servicegraphs/servicegraphs.go

Lines 57 to 59 in 251bf5a

    
           var defaultPeerAttributes = []attribute.Key{ 
        
           	semconv.PeerServiceKey, semconv.DBNameKey, semconv.DBSystemKey, 
        
           }

`processor.span_metrics.subprocessors`

tempo/modules/generator/processor/spanmetrics/config.go

Lines 59 to 62 in 251bf5a

    
           cfg.Subprocessors = make(map[Subprocessor]bool) 
        
           cfg.Subprocessors[Latency] = true 
        
           cfg.Subprocessors[Count] = true 
        
           cfg.Subprocessors[Size] = true

`storage.trace.pool`

tempo/tempodb/pool/config.go

Lines 11 to 12 in 854caca

    
           MaxWorkers: 30, 
        
           QueueDepth: 10000,

CLAassistant · 2024-05-02T11:45:53Z

All committers have signed the CLA.

WesselAtWork · 2024-05-02T12:00:18Z

Unsure if I should put the WAL config in it's own block.
The loki docs usually separate out common config into typed blocks
e.g. https://grafana.com/docs/loki/latest/configure/#s3_storage_config

They postfix these blocks with _config

WesselAtWork · 2024-05-02T12:59:49Z

docs/sources/tempo/configuration/_index.md

    # Storage and remote write configuration
    storage:

        # Path to store the WAL. Each tenant will be stored in its own subdirectory.
        path: <string>

        # Configuration for the Prometheus Agent WAL
-        wal:
+        # https://github.com/prometheus/prometheus/v2.51.2/tsdb/agent/db.go#L62-L84


Wanted to reference something (anything) here.
Feel free to dispute

WesselAtWork · 2024-05-02T13:00:07Z

docs/sources/tempo/configuration/_index.md

@@ -409,6 +419,12 @@ metrics_generator:
    # considered in metrics generation.
    # This is to filter out spans that are outdated.
    [metrics_ingestion_time_range_slack: <duration> | default = 30s]
+
+    # Timeout for generator requests (???)


Not quite sure what the correct wording should be here

WesselAtWork · 2024-05-02T13:06:32Z

docs/sources/tempo/configuration/_index.md

+        wal: <WAL Config>
+          [path: <string> | default = "/var/tempo/wal"]
+          [v2_encoding: <string> | default = snappy]
+          [search_encoding: <string> | default = none]
+          [ingestion_time_range_slack: <duration> | default = 2m]


Needed a way to show that this particular component is overriding values from the base component block. (WAL Config)

Wanted to keep it light for the future, so no comments where none would add additional value.
If there were comments we would need to keep both synchronized.

This way if you are making changes to the query-frontend you won't need to change the base block because it isn't dependant on the query-frontend
Same in reverse, if you made changes to the base WAL Config you wouldn't need to propagate them to every component that use it. (Unless you made changes to them too)

Feel free to dispute the omission of the newline breaks.

I felt a need to keep this visually distinct, but I am not married to the idea.
Might be better to keep the format consistent

WesselAtWork · 2024-05-02T13:08:05Z

docs/sources/tempo/configuration/_index.md

+# Where to store the intermediate blocks while they are being apeended to.
+# Will always join the `path` with "blocks" to generate the effective path
+# Example: "/var/tempo/wal/blocks" (ignored)
+[blocksfilepath: <ignored> | = join(.path, "/blocks")]


I think I found a bug?

I don't think this is the intended behavior

Please verify :

tempo/tempodb/wal/wal.go

Lines 75 to 81 in 251bf5a

// Setup local backend in /blocks/

p := filepath.Join(c.Filepath, blocksDir)

err = os.MkdirAll(p, os.ModePerm)

if err != nil {

return nil, err

}

c.BlocksFilepath = p

WesselAtWork · 2024-05-02T13:08:54Z

docs/sources/tempo/configuration/_index.md

+
+
 ## Overrides



Not quite sure on the placement of this (Configuration blocks) section

Feel free to dispute

WesselAtWork · 2024-05-02T13:12:51Z

docs/sources/tempo/configuration/_index.md

+    # Configuration block for the Write Ahead Log (WAL)
+    traces_storage: <WAL Config>
+
+      # Path to store the wal files.
+      # Must be set.
+      # Example: "/var/tempo/generator/traces"
+      [path: <string> | default = ""]
+


I didn't find a default value for the path is set in the code, around the metrics-generator
But If I look at the wal code, it should except if you do not set it, which feels like an oversight?
It feels like it should set a default value?

tempo/tempodb/wal/wal.go

Lines 52 to 54 in 251bf5a

if c.Filepath == "" {

return nil, fmt.Errorf("please provide a path for the WAL")

}

WesselAtWork · 2024-05-02T13:33:59Z

Just discovered something:
missing the localblocks key

tempo/modules/generator/config.go

Lines 53 to 57 in 251bf5a

    
           type ProcessorConfig struct { 
        
           	ServiceGraphs servicegraphs.Config `yaml:"service_graphs"` 
        
           	SpanMetrics   spanmetrics.Config   `yaml:"span_metrics"` 
        
           	LocalBlocks   localblocks.Config   `yaml:"local_blocks"` 
        
           }

and most of the ring keys:

tempo/modules/generator/generator_ring.go

Lines 18 to 31 in 251bf5a

    
           type RingConfig struct { 
        
           	KVStore          kv.Config     `yaml:"kvstore"` 
        
           	HeartbeatPeriod  time.Duration `yaml:"heartbeat_period"` 
        
           	HeartbeatTimeout time.Duration `yaml:"heartbeat_timeout"` 
        
           	InstanceID             string   `yaml:"instance_id"` 
        
           	InstanceInterfaceNames []string `yaml:"instance_interface_names"` 
        
           	InstanceAddr           string   `yaml:"instance_addr"` 
        
           	InstancePort           int      `yaml:"instance_port"` 
        
           	EnableInet6            bool     `yaml:"enable_inet6"` 
        
           	// Injected internally 
        
           	ListenPort int `yaml:"-"` 
        
           }

WesselAtWork · 2024-05-02T14:55:42Z

docs/sources/tempo/configuration/_index.md

@@ -330,7 +339,7 @@ metrics_generator:
        span_metrics:

            # Buckets for the latency histogram in seconds.
-            [histogram_buckets: <list of float> | default = 0.002, 0.004, 0.008, 0.016, 0.032, 0.064, 0.128, 0.256, 0.512, 1.02, 2.05, 4.10]
+            [histogram_buckets: <list of float> | default = 0.002, 0.004, 0.008, 0.016, 0.032, 0.064, 0.128, 0.256, 0.512, 1.02, 2.05, 4.10, 8.20, 16,40]


Is this correct?

tempo/modules/generator/processor/spanmetrics/config.go

Line 54 in 251bf5a

cfg.HistogramBuckets = prometheus.ExponentialBuckets(0.002, 2, 14)

Should be 14 entries correct?

joe-elliott · 2024-05-02T20:22:41Z

Howdy, @WesselAtWork. Appreciate all the details cleaned up here!

@kvrhdn is going to take a look at this in the next few days. Just didn't want you to think we were ignoring you.

docs/sources/tempo/configuration/_index.md

knylander-grafana · 2024-05-20T21:55:13Z

I love all the clean up you've done on this doc. It really helps readability. (I'll defer to developer review for this PR.)

Initial

d8360dd

Coppied from other WAL

5cb42ca

WesselAtWork changed the title ~~Initial~~ [DOC] Adding missing metric-generator keys May 2, 2024

Seperated block

3432de4

WesselAtWork commented May 2, 2024

View reviewed changes

WesselAtWork marked this pull request as ready for review May 2, 2024 13:16

WesselAtWork requested review from knylander-grafana, joe-elliott, annanay25, mdisibio, mapno, kvrhdn, zalegrala, electron0zero, ie-pham and stoewer as code owners May 2, 2024 13:16

WesselAtWork added 2 commits May 2, 2024 15:52

Added metric-generator.processor.service_graphs.peer_attributes

d67d432

Add missing span_metrics fields

e2b5b8c

WesselAtWork commented May 2, 2024

View reviewed changes

Fix wrong bool status_message

1ce273c

knylander-grafana added the type/docs Improvements or additions to documentation label May 2, 2024

knylander-grafana reviewed May 2, 2024

View reviewed changes

docs/sources/tempo/configuration/_index.md Outdated Show resolved Hide resolved

knylander-grafana reviewed May 2, 2024

View reviewed changes

docs/sources/tempo/configuration/_index.md Outdated Show resolved Hide resolved

knylander-grafana reviewed May 2, 2024

View reviewed changes

docs/sources/tempo/configuration/_index.md Outdated Show resolved Hide resolved

WesselAtWork added 4 commits May 3, 2024 10:02

Some spelling and formating

5e41652

Merge remote-tracking branch 'origin/main' into doc/metrics_generator

e86d251

Keep histogram_buckets precision to 3

afdc378

fix dimension_mappings formating

6182718

knylander-grafana mentioned this pull request May 21, 2024

[DOCS] Missing metrics_generator.traces_storage, metrics_generator.query_timeout and metrics_generator.override_ring_key #3642

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DOC] Adding missing `metric-generator` keys #3643

[DOC] Adding missing `metric-generator` keys #3643

WesselAtWork commented May 2, 2024 •

edited

CLAassistant commented May 2, 2024 •

edited

WesselAtWork commented May 2, 2024 •

edited

WesselAtWork May 2, 2024

WesselAtWork May 2, 2024

WesselAtWork May 2, 2024 •

edited

WesselAtWork May 2, 2024

WesselAtWork May 2, 2024 •

edited

WesselAtWork May 2, 2024

WesselAtWork commented May 2, 2024

WesselAtWork May 2, 2024

joe-elliott commented May 2, 2024

knylander-grafana commented May 20, 2024

	// generatorRingKey is the default key under which we store the metric-generator's ring in the KVStore.
	generatorRingKey = "metrics-generator"

	peerAttr := make([]string, 0, len(defaultPeerAttributes))
	for _, attr := range defaultPeerAttributes {
	peerAttr = append(peerAttr, string(attr))
	}

	var defaultPeerAttributes = []attribute.Key{
	semconv.PeerServiceKey, semconv.DBNameKey, semconv.DBSystemKey,
	}

	cfg.Subprocessors = make(map[Subprocessor]bool)
	cfg.Subprocessors[Latency] = true
	cfg.Subprocessors[Count] = true
	cfg.Subprocessors[Size] = true

	// Setup local backend in /blocks/
	p := filepath.Join(c.Filepath, blocksDir)
	err = os.MkdirAll(p, os.ModePerm)
	if err != nil {
	return nil, err
	}
	c.BlocksFilepath = p

	if c.Filepath == "" {
	return nil, fmt.Errorf("please provide a path for the WAL")
	}

[DOC] Adding missing metric-generator keys #3643

Are you sure you want to change the base?

[DOC] Adding missing metric-generator keys #3643

Conversation

WesselAtWork commented May 2, 2024 • edited

Sources/References

override_ring_key

override_ring_key

processor.service_graphs.peer_attributes

processor.span_metrics.subprocessors

storage.trace.pool

CLAassistant commented May 2, 2024 • edited

WesselAtWork commented May 2, 2024 • edited

WesselAtWork May 2, 2024

Choose a reason for hiding this comment

WesselAtWork May 2, 2024

Choose a reason for hiding this comment

WesselAtWork May 2, 2024 • edited

Choose a reason for hiding this comment

WesselAtWork May 2, 2024

Choose a reason for hiding this comment

WesselAtWork May 2, 2024 • edited

Choose a reason for hiding this comment

WesselAtWork May 2, 2024

Choose a reason for hiding this comment

WesselAtWork commented May 2, 2024

WesselAtWork May 2, 2024

Choose a reason for hiding this comment

joe-elliott commented May 2, 2024

knylander-grafana commented May 20, 2024

[DOC] Adding missing `metric-generator` keys #3643

[DOC] Adding missing `metric-generator` keys #3643

WesselAtWork commented May 2, 2024 •

edited

`override_ring_key`

`override_ring_key`

`processor.service_graphs.peer_attributes`

`processor.span_metrics.subprocessors`

`storage.trace.pool`

CLAassistant commented May 2, 2024 •

edited

WesselAtWork commented May 2, 2024 •

edited

WesselAtWork May 2, 2024 •

edited

WesselAtWork May 2, 2024 •

edited