-
Notifications
You must be signed in to change notification settings - Fork 30
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
Tempo query improve search performance #1048
Tempo query improve search performance #1048
Conversation
twoGBQuantity = resource.MustParse("2Gi") | ||
tenGBQuantity = resource.MustParse("10Gi") | ||
defaultServicesDuration = metav1.Duration{Duration: time.Hour * 24 * 3} | ||
defaultFindTracesConcurrentRequests = 1 |
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.
Defaulting to 1 might not be ideal. Perhaps we can default to numberofqueriers * 2
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 will keep the default to 1, here is some rationale:
Customers complain when querying large number of traces e.g. 500-1500. A single trace get from S3 can take from 2s to 8s. for 500 traces it is 500*4=2000s/60=33mins. Cutting the time in half with 2 concurrent requests does not bring any value, however 30 concurrent requests significantly improve the situation but for that customers need to scale up queriers.
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 we should use a default value which improves the current situation instead of leaving it as-is.
I like your suggestion of making the default value based on the number of queriers, maybe we can leave it 0
in the webhook and compute the default (2 * .spec.template.querier.replicas
if set to 0
) in the manifest generation?
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 above comment is for TempoStack, for TempoMonolithic we can't scale up queriers, so maybe let's go with 2 if the default max concurent requests per querier is 20?
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 have changed the default for monolithic to 2 and tempostack to queriers*2.
0203052
to
a63eb5f
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1048 +/- ##
==========================================
- Coverage 69.20% 69.14% -0.06%
==========================================
Files 110 110
Lines 7049 7059 +10
==========================================
+ Hits 4878 4881 +3
- Misses 1881 1888 +7
Partials 290 290
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Signed-off-by: Pavol Loffay <p.loffay@gmail.com>
ac969b2
to
c9e77c0
Compare
Depends on grafana/tempo#4159
Generate data