FEAT: Add Span Kind support for ES/OS #6399

Manik2708 · 2024-12-24T13:40:26Z

Which problem is this PR solving?

Fixes: #1923

Description of the changes

While querying GetOperations, operations can now be fetched with kind also. When kind kept empty, spans of all kinds are returned

How was this change tested?

Unit and E2E tests

Checklist

I have read https://github.com/jaegertracing/jaeger/blob/master/CONTRIBUTING_GUIDELINES.md
I have signed all commits
I have added unit tests for the new functionality
I have run lint and test steps successfully
- for jaeger: make lint test
- for jaeger-ui: npm run lint and npm run test

Signed-off-by: Manik2708 <[email protected]>

codecov · 2024-12-24T13:48:24Z

Codecov Report

Attention: Patch coverage is 83.78378% with 18 lines in your changes missing coverage. Please review.

Project coverage is 96.20%. Comparing base (a6616fb) to head (5b15a5f).

Files with missing lines	Patch %	Lines
plugin/storage/es/spanstore/service_operation.go	82.85%	12 Missing and 6 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6399      +/-   ##
==========================================
- Coverage   96.23%   96.20%   -0.04%     
==========================================
  Files         368      368              
  Lines       21028    21112      +84     
==========================================
+ Hits        20237    20311      +74     
- Misses        606      612       +6     
- Partials      185      189       +4

Flag	Coverage Δ
badger_v1	`10.48% <0.00%> (-0.09%)`	⬇️
badger_v2	`2.40% <0.00%> (-0.02%)`	⬇️
cassandra-4.x-v1-manual	`16.32% <0.00%> (-0.13%)`	⬇️
cassandra-4.x-v2-auto	`2.34% <0.00%> (-0.02%)`	⬇️
cassandra-4.x-v2-manual	`2.34% <0.00%> (-0.02%)`	⬇️
cassandra-5.x-v1-manual	`16.32% <0.00%> (-0.13%)`	⬇️
cassandra-5.x-v2-auto	`2.34% <0.00%> (-0.02%)`	⬇️
cassandra-5.x-v2-manual	`2.34% <0.00%> (-0.02%)`	⬇️
elasticsearch-6.x-v1	`20.51% <56.75%> (+0.30%)`	⬆️
elasticsearch-7.x-v1	`20.59% <56.75%> (+0.31%)`	⬆️
elasticsearch-8.x-v1	`20.75% <56.75%> (+0.31%)`	⬆️
elasticsearch-8.x-v2	`2.40% <0.00%> (-0.01%)`	⬇️
grpc_v1	`12.13% <0.00%> (-0.10%)`	⬇️
grpc_v2	`8.74% <0.00%> (-0.07%)`	⬇️
kafka-3.x-v1	`10.32% <0.00%> (-0.09%)`	⬇️
kafka-3.x-v2	`2.40% <0.00%> (-0.02%)`	⬇️
memory_v2	`2.40% <0.00%> (-0.01%)`	⬇️
opensearch-1.x-v1	`20.64% <56.75%> (+0.31%)`	⬆️
opensearch-2.x-v1	`20.63% <56.75%> (+0.30%)`	⬆️
opensearch-2.x-v2	`2.40% <0.00%> (-0.01%)`	⬇️
tailsampling-processor	`0.39% <0.00%> (-0.01%)`	⬇️
unittests	`95.05% <81.08%> (-0.05%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

yurishkuro

overall lgtm, but I have question on whether the whole thing could be even simpler, by treating Kind as always present (but maybe blank).

yurishkuro · 2024-12-25T21:53:10Z

plugin/storage/es/spanstore/dbmodel/model.go

+// ServiceWithKind is the JSON struct service:kind:operation documents in ElasticSearch
+type ServiceWithKind struct {
+	Service
+	Kind string `json:"spanKind"`


not sure I follow. Why wouldn't we just add Kind field to the Service struct above?

Please see the comment. As then in query for fetching spans of all kinds, we have apply another filter with kind as empty.

yurishkuro · 2024-12-25T21:55:00Z

plugin/storage/es/spanstore/writer.go

@@ -124,8 +124,9 @@ func getSpanAndServiceIndexFn(p SpanWriterParams) spanAndServiceIndexFn {
 func (s *SpanWriter) WriteSpan(_ context.Context, span *model.Span) error {
 	spanIndexName, serviceIndexName := s.spanServiceIndex(span.StartTime)
 	jsonSpan := s.spanConverter.FromDomainEmbedProcess(span)
+	kind, _ := span.GetSpanKind()


Why not add Kind field to the dbmodel.Span and not pass it around separately?

It will be present as a tag in json model also! I tried fetching kind from original span but failed because of the bool AllTagsAsFields. As it will already be present in the json model, should we resave it as a seperate kind also?

yurishkuro · 2024-12-25T21:55:55Z

plugin/storage/es/spanstore/service_operation.go

-	if !keyInCache(cacheKey, s.serviceCache) {
-		s.client().Index().Index(indexName).Type(serviceType).Id(cacheKey).BodyJson(service).Add()
-		writeCache(cacheKey, s.serviceCache)
+	if kind != model.SpanKindUnspecified {


why do we need to bifurcate? If for some reason span kind is unavailable, we can just treat it as blank string and not change the code to deal with it separately.

Please the comment #6399 (comment). If the reason seems valid then I don't think we should introduce empty kinds because that will become messy along with data of Without Kind (old data) and then in reader it might become more complex while fetching.

Manik2708 · 2024-12-26T01:53:10Z

overall lgtm, but I have question on whether the whole thing could be even simpler, by treating Kind as always present (but maybe blank).

Then we can't get old data! In old data Kind was not present. In the actual issue, when it was asked that What will happen to old data, it was answered that it should be accessible when kind is not present! Therefore I introduced a whole new struct with kind and saved the data without kind also.

Manik2708 · 2024-12-26T02:16:35Z

plugin/storage/es/spanstore/service_operation.go

+}
+
+func bucketOfOperationsToOperationsArray(searchResult *elastic.AggregationBucketFilters) ([]spanstore.Operation, error) {
+	var result []spanstore.Operation


This is concerning because we are unaware of the total length of result so not sure whether we should initialize this way or not. The best possible solution which could think of is as number of kinds are limited we can iterate through each kind fetch the total doc count of each kind, then finally create the array of total length. Will it be fine?

Signed-off-by: Manik2708 <[email protected]>

Manik2708 · 2024-12-28T16:42:43Z

@yurishkuro Comitted as per your suggestions. I tried many ways but I couldn't merge those with EmptyKinds and WithoutKinds into a single aggregation. Because both bool queries are contradictory to each other and so it is behaving in a different way. The only possible way is that we have to manage them in different aggregations which will increase complexity in the Operations. So we have to decide between complexity in writing span or fetching operations.

yurishkuro · 2024-12-28T23:40:57Z

please update the branch, I am not sure if CI is failing because of that or because of your changes. If it's the latter, how are you testing this change? Did you run e2e tests locally?

This reverts commit 8c608cd. Signed-off-by: Manik2708 <[email protected]>

Manik2708 · 2024-12-29T04:31:14Z

please update the branch, I am not sure if CI is failing because of that or because of your changes. If it's the latter, how are you testing this change? Did you run e2e tests locally?

I tried approaching with empty kind but failed as query in GetOperations is becoming more complex, hence have reverted that commit.

Manik2708 · 2025-01-02T11:08:35Z

@yurishkuro Saving empty strings in ES is not optimal. Please see: elastic/elasticsearch#7515. I have been trying various ways of employing empty or null kinds but not getting results. The probable reason is because query is a bit complex and unconventional. I think it will be better to not to keep empty and null kinds rather handelling them seperately!

yurishkuro · 2025-01-02T15:54:07Z

I don't have a strong opinion, but the issue you linked talks about searching for empty strings, which I don't think is the case for our scenario, we just need to write it.

Manik2708 · 2025-01-02T16:03:23Z

I don't have a strong opinion, but the issue you linked talks about searching for empty strings, which I don't think is the case for our scenario, we just need to write it.

But while reading we have to search for those operations also which have empty kinds or no kind! I am facing problems when fetching spans of all kinds, the search query behaves weirdly if I introduce filter of "". There are no problems in writing the service but in fetching them

yurishkuro · 2025-01-02T16:20:48Z

Searching for "all kinds" to me means not specifying any filter for the "kind" field. How would it even work if you say search for kind="" when the kind is actually not empty?

Manik2708 · 2025-01-02T19:38:21Z

Searching for "all kinds" to me means not specifying any filter for the "kind" field. How would it even work if you say search for kind="" when the kind is actually not empty?

I think there is some sort of misunderstanding of my approach. Let me first explain problems:

We want to get two fields (operatioName and kind) from our search query to ES, to achieve this, we have 3 ways:
a) Use FetchSource(true): This will fetch all the fields which is not optimal neither is avilable in our abstraction.
b) Use FetchSourceWithContext: Not available in our abstraction.
c) Composite Filters: Not possible because it will be applied to a field and kind can be absent in old data.
So I employed my approach, used the query only on service name and applied filters on kind (kinds are limited so it is possible) and used the filter name to get the kind from named buckets. But when this filter is applied to empty kind, it is not giving results. Your thoughts are absolutely correct when you said: When no kind is in query we have to just employ query on service name but we have to fetch two fields which is creating problem in just querying on service!

yurishkuro · 2025-01-02T19:46:33Z

"Not available in our abstraction" is an odd argument since we own the abstraction and can change it at will. Re fetchsource, what is the source here - is it the whole span? Or do we write separate entries just for service/operation?

Manik2708 · 2025-01-02T20:26:49Z

"Not available in our abstraction" is an odd argument since we own the abstraction and can change it at will. Re fetchsource, what is the source here - is it the whole span? Or do we write separate entries just for service/operation?

Service, Operation and Kind (if present) but have to investigate the indices whether it is extracting any other information.

FEAT: Add Span Kind support for ES/OS

502dd38

Signed-off-by: Manik2708 <[email protected]>

Manik2708 requested a review from a team as a code owner December 24, 2024 13:40

Manik2708 requested a review from jkowall December 24, 2024 13:40

Merge branch 'main' into es-kind

02bff1c

dosubot bot added area/storage enhancement labels Dec 24, 2024

yurishkuro reviewed Dec 25, 2024

View reviewed changes

Manik2708 requested a review from yurishkuro December 26, 2024 02:05

Manik2708 commented Dec 26, 2024

View reviewed changes

Manik2708 added 4 commits December 26, 2024 22:38

Merge branch 'main' into es-kind

b0da654

conflicts resolved

6570013

Signed-off-by: Manik2708 <[email protected]>

conflicts resolved

1cba60a

Signed-off-by: Manik2708 <[email protected]>

clean up

8c608cd

Signed-off-by: Manik2708 <[email protected]>

Manik2708 added 2 commits December 29, 2024 09:58

Revert "clean up"

bc55f6a

This reverts commit 8c608cd. Signed-off-by: Manik2708 <[email protected]>

Merge branch 'main' into es-kind

5b15a5f

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FEAT: Add Span Kind support for ES/OS #6399

FEAT: Add Span Kind support for ES/OS #6399

Manik2708 commented Dec 24, 2024

codecov bot commented Dec 24, 2024 •

edited

Loading

yurishkuro left a comment

yurishkuro Dec 25, 2024

Manik2708 Dec 27, 2024

yurishkuro Dec 25, 2024

Manik2708 Dec 26, 2024 •

edited

Loading

yurishkuro Dec 25, 2024

Manik2708 Dec 26, 2024

Manik2708 commented Dec 26, 2024 •

edited

Loading

Manik2708 Dec 26, 2024 •

edited

Loading

Manik2708 commented Dec 28, 2024 •

edited

Loading

yurishkuro commented Dec 28, 2024

Manik2708 commented Dec 29, 2024 •

edited

Loading

Manik2708 commented Jan 2, 2025 •

edited

Loading

yurishkuro commented Jan 2, 2025

Manik2708 commented Jan 2, 2025

yurishkuro commented Jan 2, 2025

Manik2708 commented Jan 2, 2025 •

edited

Loading

yurishkuro commented Jan 2, 2025

Manik2708 commented Jan 2, 2025 •

edited

Loading

FEAT: Add Span Kind support for ES/OS #6399

Are you sure you want to change the base?

FEAT: Add Span Kind support for ES/OS #6399

Conversation

Manik2708 commented Dec 24, 2024

Which problem is this PR solving?

Description of the changes

How was this change tested?

Checklist

codecov bot commented Dec 24, 2024 • edited Loading

Codecov Report

yurishkuro left a comment

Choose a reason for hiding this comment

yurishkuro Dec 25, 2024

Choose a reason for hiding this comment

Manik2708 Dec 27, 2024

Choose a reason for hiding this comment

yurishkuro Dec 25, 2024

Choose a reason for hiding this comment

Manik2708 Dec 26, 2024 • edited Loading

Choose a reason for hiding this comment

yurishkuro Dec 25, 2024

Choose a reason for hiding this comment

Manik2708 Dec 26, 2024

Choose a reason for hiding this comment

Manik2708 commented Dec 26, 2024 • edited Loading

Manik2708 Dec 26, 2024 • edited Loading

Choose a reason for hiding this comment

Manik2708 commented Dec 28, 2024 • edited Loading

yurishkuro commented Dec 28, 2024

Manik2708 commented Dec 29, 2024 • edited Loading

Manik2708 commented Jan 2, 2025 • edited Loading

yurishkuro commented Jan 2, 2025

Manik2708 commented Jan 2, 2025

yurishkuro commented Jan 2, 2025

Manik2708 commented Jan 2, 2025 • edited Loading

yurishkuro commented Jan 2, 2025

Manik2708 commented Jan 2, 2025 • edited Loading

codecov bot commented Dec 24, 2024 •

edited

Loading

Manik2708 Dec 26, 2024 •

edited

Loading

Manik2708 commented Dec 26, 2024 •

edited

Loading

Manik2708 Dec 26, 2024 •

edited

Loading

Manik2708 commented Dec 28, 2024 •

edited

Loading

Manik2708 commented Dec 29, 2024 •

edited

Loading

Manik2708 commented Jan 2, 2025 •

edited

Loading

Manik2708 commented Jan 2, 2025 •

edited

Loading

Manik2708 commented Jan 2, 2025 •

edited

Loading