-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Use a single index with wildcard in Elasticsearch reader #1969
Conversation
Signed-off-by: Pavol Loffay <[email protected]>
Signed-off-by: Pavol Loffay <[email protected]>
Codecov Report
@@ Coverage Diff @@
## master #1969 +/- ##
==========================================
- Coverage 96.99% 96.99% -0.01%
==========================================
Files 203 203
Lines 10061 10034 -27
==========================================
- Hits 9759 9732 -27
Misses 264 264
Partials 38 38
Continue to review full report at Codecov.
|
Signed-off-by: Pavol Loffay <[email protected]>
Signed-off-by: Pavol Loffay <[email protected]>
Signed-off-by: Pavol Loffay <[email protected]>
@jaegertracing/elasticsearch we are looking for volunteers to test this functionality especially if the query performance changed. Any feedback is appreciated. The docker images are listed in the PR description. |
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.
Should we also change the esCleaner.py
script as part of this PR and allow users to specify a max TTL for span data?
nsConfig.MaxSpanAge, | ||
"The maximum lookback for spans in Elasticsearch") | ||
time.Hour*72, | ||
"(deprecated) The maximum lookback for spans in Elasticsearch. Now all indices are searched.") |
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.
This seems confusing. Can we add the message Now all indices are searched
in the parenthesis?
I am not sure if I follow you. The purpose of |
@pavolloffay Sorry I didn't frame that properly.
Yes. What I meant was that should we modify the |
There is no change in index name format in this PR. The writer still writes to daily indices. |
@jkandasa run performance tests to measure query time with this patch. The results prove increased query time by on average 300-400%. Therefore I am closing this PR. https://docs.google.com/spreadsheets/d/1rwTzDvnHHhssNdZibhFuHBZDf6njt6b1DrvRiJrkt14/edit?usp=sharing |
Resolves #1361
Use a wildcard (
jaeger-span-*
) in queries instead of the concrete list of indices (jaeger-span-2018-12-24
,jaeger-span-2018-12-25
...).The motivation for this change:
--es.max-span-age
parameterrollover lookback
option (similar to--es-max-span-age
) https://medium.com/jaegertracing/using-elasticsearch-rollover-to-manage-indices-8b3d0c77915dPossible negative impacts:
Docker images to test:
GetTrace
,GetServices
andGetOperatios
used--es.max-span-age
to compose a list of historical indices for the query - now query all indices via the wildcardFidTraceIDs
andFindTraces
use times from query parameters to compose the list of historical indices - now query all indices but they use timestamp range query.