-
Notifications
You must be signed in to change notification settings - Fork 489
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[DOC] Adding missing metric-generator
keys
#3643
base: main
Are you sure you want to change the base?
Conversation
Unsure if I should put the WAL config in it's own block. They postfix these blocks with |
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] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
|
||
|
||
## Overrides | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not quite sure on the placement of this (Configuration blocks) section
Feel free to dispute
# 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 = ""] | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
Lines 52 to 54 in 251bf5a
if c.Filepath == "" { | |
return nil, fmt.Errorf("please provide a path for the WAL") | |
} |
Just discovered something: tempo/modules/generator/config.go Lines 53 to 57 in 251bf5a
and most of the ring keys: tempo/modules/generator/generator_ring.go Lines 18 to 31 in 251bf5a
|
@@ -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] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this correct?
cfg.HistogramBuckets = prometheus.ExponentialBuckets(0.002, 2, 14) |
Should be 14 entries correct?
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. |
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.) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for this work! 🙏🏻
Sorry it took a while to get through it, overall changes look good I've just left some smaller comments. Once those are addressed this is good to go 🙂
# Maximum (???) (lifespan for any given block?) | ||
[max_block_duration: <duration> | default = 1m] | ||
|
||
# Maximum block size before it is closed (?) | ||
[max_block_bytes: <uint64> | default = 500000000] | ||
|
||
# Maximum (???) | ||
[complete_block_timeout: <duration> | default = 1h] | ||
|
||
# (Unused?) | ||
# Max number of "live" traces | ||
[max_live_traces: <uint64>] | ||
|
||
# Whether server spans should be filtered in or not. | ||
# true means keep the server kinds (???) | ||
# false means drop the server kinds (???) | ||
[filter_server_spans: <bool> | default = true] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Help with these descriptions please
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Happy to try to help. What do you mean by server kinds?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tempo/modules/generator/processor/localblocks/processor.go
Lines 886 to 920 in 56a0b8c
// filterBatches to only root spans or kind==server. Does not modify the input | |
// but returns a new struct referencing the same input pointers. Returns nil | |
// if there were no matching spans. | |
func filterBatches(batches []*v1.ResourceSpans) []*v1.ResourceSpans { | |
keep := make([]*v1.ResourceSpans, 0, len(batches)) | |
for _, batch := range batches { | |
var keepSS []*v1.ScopeSpans | |
for _, ss := range batch.ScopeSpans { | |
var keepSpans []*v1.Span | |
for _, s := range ss.Spans { | |
if s.Kind == v1.Span_SPAN_KIND_SERVER || len(s.ParentSpanId) == 0 { | |
keepSpans = append(keepSpans, s) | |
} | |
} | |
if len(keepSpans) > 0 { | |
keepSS = append(keepSS, &v1.ScopeSpans{ | |
Scope: ss.Scope, | |
Spans: keepSpans, | |
}) | |
} | |
} | |
if len(keepSS) > 0 { | |
keep = append(keep, &v1.ResourceSpans{ | |
Resource: batch.Resource, | |
ScopeSpans: keepSS, | |
}) | |
} | |
} | |
return keep | |
} |
It's the span kind(?) client/server kinds I think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My suggestions would be:
# How often to run flush checks (??)
[flush_check_period: <duration> | default = 10s]
How often to run the flush loop to cut idle traces and blocks
# After this idle period, a trace is closed. (??)
[trace_idle_period: <duration> | default = 10s]
Duration after which to consider a trace complete if no spans have been received
(this is copied from the flag definition)
# Maximum (???) (lifespan for any given block?)
[max_block_duration: <duration> | default = 1m]
Maximum duration which the head block can be appended to before cutting it
# Maximum block size before it is closed (?)
[max_block_bytes: <uint64> | default = 500000000]
Maximum size of the head block before cutting it
# Maximum (???)
[complete_block_timeout: <duration> | default = 1h]
Duration to keep blocks in the ingester after they have been flushed
# (Unused?)
# Max number of "live" traces
[max_live_traces: <uint64>]
Maximum amount of live traces, if this is exceeded traces will be dropped with reason
live_traces_exceeded
.
# Whether server spans should be filtered in or not.
# true means keep the server kinds (???)
# false means drop the server kinds (???)
[filter_server_spans: <bool> | default = true]
This code iterates through all spans and only keeps spans that either:
- have span.kind == SPAN_KIND_SERVER
- don't have a parent span
If enabled, only parent spans or spans with SpanKind server will be retained
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Out of curiosity, why only retain parents and Server
spans by default?
What about other spans or Client
types?
# Where to store the intermediate blocks while they are being appended to. | ||
# Will always join the `path` with "blocks" to generate the effective path | ||
# Example: "/var/tempo/wal/blocks" (ignored) | ||
[blocksfilepath: <ignored> | = join(.path, "/blocks")] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I found a bug?
I described what the code does here, but I don't think this is the intended behaviour...
Please verify :
Lines 70 to 75 in 3a4f309
p := filepath.Join(c.Filepath, blocksDir) | |
err = os.MkdirAll(p, os.ModePerm) | |
if err != nil { | |
return nil, err | |
} | |
c.BlocksFilepath = p |
I think it's supposed to look like this (but with c.BlocksFilepath == ""
):
Lines 56 to 67 in 3a4f309
// The /completed/ folder is now obsolete and no new data is written, | |
// but it needs to be cleared out one last time for any files left | |
// from a previous version. | |
if c.CompletedFilepath == "" { | |
completedFilepath := filepath.Join(c.Filepath, completedDir) | |
err = os.RemoveAll(completedFilepath) | |
if err != nil { | |
return nil, err | |
} | |
c.CompletedFilepath = completedFilepath | |
} |
Should I pull master? Seems it broke on the linting step |
Apologies, I'd only seen the last commit and not the full PR.
Try rebasing on |
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
override_ring_key
tempo/modules/generator/config.go
Line 50 in 251bf5a
tempo/modules/generator/config.go
Lines 18 to 19 in 251bf5a
processor.service_graphs.peer_attributes
tempo/modules/generator/processor/servicegraphs/config.go
Lines 54 to 57 in 251bf5a
tempo/modules/generator/processor/servicegraphs/servicegraphs.go
Lines 57 to 59 in 251bf5a
processor.span_metrics.subprocessors
This one did not work
tempo/modules/generator/processor/spanmetrics/config.go
Lines 59 to 62 in 251bf5a
storage.trace.pool
tempo/tempodb/pool/config.go
Lines 11 to 12 in 854caca