[storage] Add Elasticsearch data stream support #7768
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #7768 +/- ##
==========================================
- Coverage 95.53% 95.44% -0.10%
==========================================
Files 307 308 +1
Lines 15911 16015 +104
==========================================
+ Hits 15201 15285 +84
- Misses 558 570 +12
- Partials 152 160 +8
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Part of jaegertracing#4708 Adds opt-in support for Elasticsearch data streams to replace traditional date-based indices with automatic lifecycle management. Signed-off-by: SoumyaRaikwar <[email protected]>
SoumyaRaikwar
force-pushed
the
feature/es-datastream-support
branch
from
c5f1ae9 to
1b79e3a
Compare
|
PR implements data stream templates aligned with @Manik2708's design doc:
Templates are ready for review. |
|
Thanks @SoumyaRaikwar! But I would like to suggest a little break. The design doc is not ready for implementation yet. Currently it lacks evidence, we might have to manually update the templates and then see how it behaves. Moreover we have to see whether we are missing something over backward compatibility. Currently it has my research from docs but I still have to test and provide the steps for reviewer. If you can help with this, then I would be very grateful to you. |
|
@Manik2708, I've reviewed your proposal and the current template implementation. I'm keeping my PR as draft for now. I can help with the manual testing you mentioned. I'll set up a local ES cluster, create the ingest pipelines, apply the data stream templates, and verify the behavior. I'll document the steps and results here for the design doc evidence. Let me know if there's a specific scenario you'd like me to prioritize |
|
I've completed initial testing and evidence gathering. Here's what I verified: Design Alignment Confirmed
Ingest Pipeline TestedCreated and tested
Manual Verification EvidencePosted test document to data stream:
Full evidence report: https://gist.github.com/SoumyaRaikwar/519a98bcc81dc2df04308ae4a66b702b Next StepsReady to work on:
Which should I prioritize? |
…ence Signed-off-by: SoumyaRaikwar <[email protected]>
| UseILM bool `mapstructure:"use_ilm"` | ||
| // UseDataStream, if set to true, will use Elasticsearch data streams for storing traces. | ||
| // This requires Elasticsearch 7.9+. | ||
| UseDataStream bool `mapstructure:"use_data_stream"` |
There was a problem hiding this comment.
datastream is going to be the default strategy, we don't need this
There was a problem hiding this comment.
Got it - data streams will be the default strategy for ES 8+. I'll remove the config flag and make data streams the standard path for span storage.
| Index(index string) IndexService | ||
| Type(typ string) IndexService | ||
| Id(id string) IndexService | ||
| OpType(opType string) IndexService |
There was a problem hiding this comment.
what's this? what do we want to do?
There was a problem hiding this comment.
The OpType(opType string) method is required for data stream writes. ES data streams only accept documents with op_type=create (not index). Without this, we get 400 errors from ES. This is why I added it to the IndexService interface and mocks.
There was a problem hiding this comment.
There was a problem hiding this comment.
then why to add in interface? its a part of *elastic.BulkRequest, can't we add op type in Add method or perhaps directly in WrapESIndexService?
| @@ -0,0 +1,167 @@ | |||
| { | |||
There was a problem hiding this comment.
we don't need this as a seperate template, I think only v8 templates needs to be changed
There was a problem hiding this comment.
Understood - I'll remove all non-span data stream implementations:
- Remove
jaeger-ds-service-8.json - Remove
jaeger-ds-dependencies-8.json - Remove
jaeger-ds-sampling-8.json - Keep only
jaeger-ds-span-8.json
There was a problem hiding this comment.
my main point was to change the existing v8 template, so I don't think we even need jaeger-ds-span-8.json
| @@ -0,0 +1,23 @@ | |||
| { | |||
There was a problem hiding this comment.
we want only spans in datastream as for now, no need of services etc
There was a problem hiding this comment.
Understood! I'll remove all non-span data stream files:
- jaeger-ds-service-8.json
- jaeger-ds-dependencies-8.json
- jaeger-ds-sampling-8.json
- Keep only jaeger-ds-span-8.json
Will also remove the corresponding code from samplingstore, depstore, and service_operation stores. Working on it now.
| UseILM bool `mapstructure:"use_ilm"` | ||
| // UseDataStream, if set to true, will use Elasticsearch data streams for storing traces. | ||
| // This requires Elasticsearch 7.9+. | ||
| UseDataStream bool `mapstructure:"use_data_stream"` |
There was a problem hiding this comment.
I would suggest using featuregates for dual look up in reader
There was a problem hiding this comment.
Great idea! I'll implement it with feature gates for the dual lookup logic.
To clarify the approach:
- Keep
UseDataStreamas an internal implementation detail (not exposed in config) - Use feature gates to control reader behavior (search data streams vs regular indices)
- For ES 8+, data streams will be the default write path
This way we can safely migrate without breaking existing deployments. Does this align with your vision?
| @@ -0,0 +1,116 @@ | |||
| # Elasticsearch Data Streams in Jaeger | |||
There was a problem hiding this comment.
I don't know whether it should be added here or not. I would suggest to add this in Google doc first and get it approved there by the maintainers. For documentation I am more concerned for the modification of ILM/Templates as that information would be of utmost importance for the users!
There was a problem hiding this comment.
done i have updated it here https://gist.github.com/SoumyaRaikwar/33b696ae9ce488c99c165f6301adf7e1 so maintainers could review it
|
I've updated the design document and the Gist with the proposed Index Lifecycle Management (ILM) policy for data streams. Updates include:
Please review the updated proposal. Once approved, I can proceed with applying the ILM policy to the templates. |
Implement jaeger.es.readLegacyWithDataStream feature gate and enhance ILM policy with forcemerge. Signed-off-by: SoumyaRaikwar <[email protected]>
SoumyaRaikwar
force-pushed
the
feature/es-datastream-support
branch
from
ca6e88d to
3cdd9aa
Compare
…Fielddata error - Updated ES 8 index patterns to include both legacy and data stream names - Resolved Fielddata error for serviceName by ensuring mappings apply to data streams - Fixed writer_test.go mock expectations and addressed linting issues (revive, testifylint) Signed-off-by: SoumyaRaikwar <[email protected]>
Signed-off-by: SoumyaRaikwar <[email protected]>
Signed-off-by: SoumyaRaikwar <[email protected]>
Metrics Comparison SummaryTotal changes across all snapshots: 0 Detailed changes per snapshotsummary_metrics_snapshot_cassandra📊 Metrics Diff SummaryTotal Changes: 0
summary_metrics_snapshot_cassandra📊 Metrics Diff SummaryTotal Changes: 0
|
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
Co-authored-by: graphite-app[bot] <96075541+graphite-app[bot]@users.noreply.github.com> Signed-off-by: Soumya Raikwar <[email protected]>
Signed-off-by: SoumyaRaikwar <[email protected]>
|
@yurishkuro i have updated PR to reflect changes as per doc, could you review? |
Signed-off-by: SoumyaRaikwar <[email protected]>
Signed-off-by: SoumyaRaikwar <[email protected]>
Signed-off-by: SoumyaRaikwar <[email protected]>
2 participants
[storage] Add Elasticsearch data stream support
Ref: #4708
Part of #4708
Which problem is this PR solving?
Adds comprehensive support for Elasticsearch data streams in Jaeger storage. Data streams provide automatic rollover, simplified scaling, and optimized lifecycle management for time-series data, replacing traditional date-suffixed indices.
Description of the changes
jaeger-ds-*naming convention (e.g.,jaeger-ds-span) for all data streams. This ensures strict isolation, preventing legacy wildcard searches (jaeger-span-*) from accidentally picking up new Data Stream data.OpType("create")and remove date suffixes when Data Streams are enabled.startTimeto the mandatory@timestampfield required by Data Streams.How was this change tested?
SpanStore, SamplingStore, and DependencyStore.tracegen.Checklist
make lint testPASS