feat: include unscoped queries and default the batch size to 7 #4436
Conversation
javiermolinar
requested review from
joe-elliott,
mdisibio,
mapno,
yvrhdn,
zalegrala,
electron0zero,
ie-pham and
stoewer
as code owners
| @@ -114,12 +114,17 @@ func (c weightRequestWare) setTraceQLWeight(req Request) { | |||
| if c.Op != traceql.OpNone { | |||
| conditions++ | |||
| } | |||
|
|
|||
| if c.Attribute.Intrinsic == traceql.IntrinsicNone && c.Attribute.Scope == traceql.AttributeScopeNone { | |||
There was a problem hiding this comment.
what are we trying to catch here?
There was a problem hiding this comment.
oh, you are looking for unscoped attributes. it may be safer to test spanRequest.AllConditions at the bottom?
i'm fine with this, but perhaps do a bit of testing before we merge?
There was a problem hiding this comment.
Do you mean something like?:
if heavyQuery && !spanRequest.AllConditions{
req.SetWeight(c.weights.TraceQLSearchWeight + 1)
}There was a problem hiding this comment.
apologies, i meant to edit this comment and got sidetracked. AllConditions will only be true if its a single spanset selector will all conditions &&'ed. It will be true even with unscoped attributes, so it will be true on both of these:
{ span.foo = "bar" && resource.baz = "bat" }
{ .foo = "bar" && .baz = "bat" }
If AllConditions is false -OR- there are unscoped attributes the querier is a heavier query b/c it cannot be evaluated in the fetch layer and requires the engine.
It's possible that both unscoped attributes and AllConditions false should result in TraceQLSerachWeight + 1 but would need some testing.
| @@ -114,12 +114,17 @@ func (c weightRequestWare) setTraceQLWeight(req Request) { | |||
| if c.Op != traceql.OpNone { | |||
| conditions++ | |||
| } | |||
|
|
|||
| if c.Attribute.Intrinsic == traceql.IntrinsicNone && c.Attribute.Scope == traceql.AttributeScopeNone { | |||
There was a problem hiding this comment.
oh, you are looking for unscoped attributes. it may be safer to test spanRequest.AllConditions at the bottom?
i'm fine with this, but perhaps do a bit of testing before we merge?
|
Does this impact configuration or how a user would run queries? In other words, are there any changes in this PR that would impact how a user interacts or configures Tempo? If so, we should consider updating the docs. |
What this PR does:
It checks for unscoped attributes to weigh the request.
It sets the default batch size to 7. Our tests have proved that a batch size of less than 7 in combination with weighted requests introduces a latency penalty due to contention.
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX]