[feat][es/os] Add missing link and scope fields in dbmodel#8473
[feat][es/os] Add missing link and scope fields in dbmodel#8473Manik2708 wants to merge 31 commits into
Conversation
Signed-off-by: Manik Mehta <mehtamanik96@gmail.com>
|
Some notes to the reviewer:
|
There was a problem hiding this comment.
Pull request overview
This PR addresses ES/OS v2 trace model gaps by adding storage/roundtrip support for OpenTelemetry Instrumentation Scope attributes and Span Link fields (attributes, tracestate, flags), and updates ES query construction so these new fields are searchable (including compatibility handling for older indices).
Changes:
- Extend ES v2
dbmodelto includeSpan.ScopeTags/ScopeTagandReference.TraceState/Flags/Tags/Tag. - Update OTLP <-> DB model translation to preserve scope attributes and link fields.
- Expand ES tag querying fixtures/logic and enable previously skipped integration tests for scope/link attributes.
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/storage/v2/elasticsearch/tracestore/to_dbmodel.go | Persist scope attributes on spans and link tracestate/flags/attributes on references. |
| internal/storage/v2/elasticsearch/tracestore/from_dbmodel.go | Restore scope attributes and link tracestate/flags/attributes back into OTLP traces. |
| internal/storage/v2/elasticsearch/tracestore/fixtures/otel_traces_01.json | Add scope attributes and link tracestate/flags/tags to OTLP fixture. |
| internal/storage/v2/elasticsearch/tracestore/fixtures/es_01.json | Update ES span fixture to include scopeTags and reference link fields. |
| internal/storage/v2/elasticsearch/tracestore/fixtures/es_01_string_tags.json | Same as above for string-encoded tag values. |
| internal/storage/v2/elasticsearch/tracestore/core/reader.go | Expand tag query fields to include scope/reference tags; add ignore_unmapped for new nested paths; merge tags for new fields. |
| internal/storage/v2/elasticsearch/tracestore/core/fixtures/query_01.json | Update expected ES query JSON to include scope/reference tag queries. |
| internal/storage/v2/elasticsearch/tracestore/core/fixtures/query_02.json | Same for regex query fixture. |
| internal/storage/v2/elasticsearch/tracestore/core/fixtures/query_03.json | Same for escaped-regex query fixture. |
| internal/storage/v2/elasticsearch/tracestore/core/dbmodel/model.go | Add new dbmodel fields for scope tags and link fields. |
| internal/storage/v1/elasticsearch/mappings/jaeger-span-6.json | Add mappings for scopeTag/scopeTags and reference traceState/flags/tags. |
| internal/storage/v1/elasticsearch/mappings/jaeger-span-7.json | Same mapping updates for ES7. |
| internal/storage/v1/elasticsearch/mappings/jaeger-span-8.json | Same mapping updates for ES8. |
| internal/storage/v1/elasticsearch/mappings/fixtures/jaeger-span-6.json | Update mapping fixture snapshot. |
| internal/storage/v1/elasticsearch/mappings/fixtures/jaeger-span-7.json | Update mapping fixture snapshot. |
| internal/storage/v1/elasticsearch/mappings/fixtures/jaeger-span-8.json | Update mapping fixture snapshot. |
| internal/storage/integration/capabilities/capabilities.go | Stop skipping scope/link attribute integration tests for ES/OS. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 20 out of 20 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #8473 +/- ##
==========================================
+ Coverage 96.56% 96.57% +0.01%
==========================================
Files 331 332 +1
Lines 17566 17681 +115
==========================================
+ Hits 16963 17076 +113
- Misses 454 455 +1
- Partials 149 150 +1
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:
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 20 out of 20 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
I feel like this is a very unfriendly approach for the user. My preference would be
|
Just two questions: where should that script be placed and how it should be pointed for the users (I mean should we provide the PR link in the error?) |
| // Check for scopeTag (Scope attributes) | ||
| if _, ok = properties["scopeTag"]; !ok { | ||
| return fmt.Errorf("template %q is missing 'scopeTag' field; please update mappings", templateName) | ||
| } |
Signed-off-by: Manik Mehta <mehtamanik96@gmail.com>
Signed-off-by: Manik Mehta <mehtamanik96@gmail.com>
|
@yurishkuro a gentle reminder to review the pr |
Signed-off-by: Manik Mehta <mehtamanik96@gmail.com>
Signed-off-by: Manik Mehta <mehtamanik96@gmail.com>
|
@Manik2708 I'll help with reviewing this PR and have the following suggestions to break it down to make it easier to review:
Try to open prequel PRs that set up the preparation for this change before making the breaking change itself. |
Hey, thanks for the reply! My reason of raising one PR as a whole was to test manually on some points, for example turning off the |
I don't think we need e2e tests for the migration script. A manual test should be sufficient. You can spin up an instance of ES and run the migration script it against it and add the results to the PR description. |
@mahadzaryab1 I am thinking of 4 PRs as following:
|
lgtm! |
## Which problem is this PR solving? - Pr-requisite of: #8473 ## Description of the changes - Required changes in index templates as one of the parts of the PR ## How was this change tested? - Unit Tests ## Checklist - [x] I have read https://github.com/jaegertracing/jaeger/blob/main/CONTRIBUTING_GUIDELINES.md - [x] I have signed all commits - [x] I have added unit tests for the new functionality - [x] I have run lint and test steps successfully: `make lint test` ## AI Usage in this PR (choose one) See [AI Usage Policy](https://github.com/jaegertracing/jaeger/blob/main/CONTRIBUTING_GUIDELINES.md#ai-usage-policy). - [x] **None**: No AI tools were used in creating this PR - [ ] **Light**: AI provided minor assistance (formatting, simple suggestions) - [ ] **Moderate**: AI helped with code generation or debugging specific parts - [ ] **Heavy**: AI generated most or all of the code changes Signed-off-by: Manik Mehta <mehtamanik96@gmail.com>
…ing#8643) ## Which problem is this PR solving? - Pr-requisite of: jaegertracing#8473 ## Description of the changes - Required changes in index templates as one of the parts of the PR ## How was this change tested? - Unit Tests ## Checklist - [x] I have read https://github.com/jaegertracing/jaeger/blob/main/CONTRIBUTING_GUIDELINES.md - [x] I have signed all commits - [x] I have added unit tests for the new functionality - [x] I have run lint and test steps successfully: `make lint test` ## AI Usage in this PR (choose one) See [AI Usage Policy](https://github.com/jaegertracing/jaeger/blob/main/CONTRIBUTING_GUIDELINES.md#ai-usage-policy). - [x] **None**: No AI tools were used in creating this PR - [ ] **Light**: AI provided minor assistance (formatting, simple suggestions) - [ ] **Moderate**: AI helped with code generation or debugging specific parts - [ ] **Heavy**: AI generated most or all of the code changes Signed-off-by: Manik Mehta <mehtamanik96@gmail.com> Signed-off-by: Victor Chidera Obiezue <obiezuechidera@gmail.com>
|
too many merge conflicts |
|
@yurishkuro #8668 is a part of this PR which is blocked due #8691, once we have backward compatibility tests running, it will be easier to merge #8668 and then this PR will be unblocked |
Which problem is this PR solving?
Description of the changes
Migration Guide: Manual Mapping Updates for Scope and Link Attributes
The first step of the migration guide is optional for every user but second step is mandatory for the users who have turned
CreateIndexTemplatesoff and are managing the mappings manually.1. Update Existing Indices
You can apply these updates to your current active index using the
_mappingAPI.💡 Note: Unlike the Template API, the
_mappingAPI is a merge operation by default; existing fields will not be affected. We includedynamic_templatesso that new attributes appearing in the current index are correctly mapped.Note: Replace
jaeger-span-YYYY-MM-DDwith your actual current index name.The reason why it is optional is because if you will not do this then you will not be able to see the changes even after upgrading jaeger because jaeger will update the template not mapping which will be reflected in the mappings created in the future. So do this only if you want changes in your present mappings too.
2. Update Index Templates (For Future Indices)
This guide provides the specific Elasticsearch API calls required for users who manage their own mappings (
CreateIndexTemplates=false). These updates enable storage for the new OpenTelemetry Instrumentation Scope and Span Link attributes.operationName,startTime, etc.).You must MERGE these changes into your existing template definition.
Steps to Update:
mappingssection of yourfull_template.json:{ "dynamic_templates": [ /* Add these to your existing dynamic_templates list */ { "scope_tags_map": { "path_match": "scopeTag.*", "mapping": { "type": "keyword", "ignore_above": 256 } } } ], "properties": { /* Add these to your existing root properties */ "scopeTag": { "type": "object" }, "scopeTags": { "type": "nested", "dynamic": false, "properties": { "key": { "type": "keyword", "ignore_above": 256 }, "value": { "type": "keyword", "ignore_above": 256 }, "type": { "type": "keyword", "ignore_above": 256 } } }, /* Update your existing references block with these new fields */ "references": { "type": "nested", "properties": { /* ... keep your existing refType, traceID, spanID ... */ "traceState": { "type": "keyword", "ignore_above": 256 }, "flags": { "type": "integer" }, "tags": { "type": "nested", "dynamic": false, "properties": { "key": { "type": "keyword", "ignore_above": 256 }, "value": { "type": "keyword", "ignore_above": 256 }, "type": { "type": "keyword", "ignore_above": 256 } } } } } } }Verification
Verify the update by checking the mapping of any index:
How was this change tested?
Mannual Testing
This testing was done to ensure that the query changes are running fine on old data. For this, 1000 traces were sent to jaeger and then a query related to attributes was run to see the number of traces, the result was:
Then this same query was run after changing the branch, rebuilding the jaeger and resending the 1000 traces
The change of output from 1 to 2 shows that query ran fine without any error on old data too.
Checklist
make lint testAI Usage in this PR (choose one)
See AI Usage Policy.