Skip to content

[ES][v1] Change the DB Tag value from string to any type #6998

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.

Sign up for GitHub

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

Merged
merged 4 commits into from
Apr 10, 2025
+190 −71
Merged

[ES][v1] Change the DB Tag value from string to any type #6998

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
2950d1e
[ES][v1] Change the DB Tag value from string to any type
Manik2708 Apr 7, 2025
e01b9f1
codecov
Manik2708 Apr 7, 2025
1940b1b
Merge branch 'main' into tag-v1
Manik2708 Apr 7, 2025
c5e4193
Merge branch 'main' into tag-v1
Manik2708 Apr 8, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 6 additions & 6 deletions internal/storage/v1/elasticsearch/spanstore/fixtures/es_01.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,17 +32,17 @@
{
"key": "peer.ipv4",
"type": "int64",
"value": "23456"
"value": 23456
},
{
"key": "error",
"type": "bool",
"value": "true"
"value": true
},
{
"key": "temperature",
"type": "float64",
"value": "72.5"
"value": 72.5
},
{
"key": "blob",
Expand All @@ -57,7 +57,7 @@
{
"key": "event",
"type": "int64",
"value": "123415"
"value": 123415
}
]
},
Expand All @@ -78,12 +78,12 @@
{
"key": "peer.ipv4",
"type": "int64",
"value": "23456"
"value": 23456
},
{
"key": "error",
"type": "bool",
"value": "true"
"value": true
}
]
}
Expand Down
13 changes: 9 additions & 4 deletions internal/storage/v1/elasticsearch/spanstore/from_domain.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,9 +119,14 @@ func (fd FromDomain) convertProcess(process *model.Process) dbmodel.Process {
}

func convertKeyValue(kv model.KeyValue) dbmodel.KeyValue {
return dbmodel.KeyValue{
Key: kv.Key,
Type: dbmodel.ValueType(strings.ToLower(kv.VType.String())),
Value: kv.AsString(),
outKv := dbmodel.KeyValue{
Key: kv.Key,
Type: dbmodel.ValueType(strings.ToLower(kv.VType.String())),
}
if kv.GetVType() == model.BinaryType {
outKv.Value = kv.AsString()
} else {
outKv.Value = kv.Value()
}
return outKv
}
17 changes: 5 additions & 12 deletions internal/storage/v1/elasticsearch/spanstore/from_domain_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,12 +32,9 @@ func TestFromDomainEmbedProcess(t *testing.T) {
converter := NewFromDomain(false, nil, ":")
embeddedSpan := converter.FromDomainEmbedProcess(&span)

var expectedSpan dbmodel.Span
require.NoError(t, json.Unmarshal(jsonStr, &expectedSpan))

testJSONEncoding(t, i, jsonStr, embeddedSpan)

CompareJSONSpans(t, &expectedSpan, embeddedSpan)
CompareJSONSpans(t, jsonStr, embeddedSpan)
})
}
}
Expand Down Expand Up @@ -112,19 +109,19 @@ func TestConvertKeyValueValue(t *testing.T) {
}{
{
kv: model.Bool(key, true),
expected: dbmodel.KeyValue{Key: key, Value: "true", Type: "bool"},
expected: dbmodel.KeyValue{Key: key, Value: true, Type: "bool"},
},
{
kv: model.Bool(key, false),
expected: dbmodel.KeyValue{Key: key, Value: "false", Type: "bool"},
expected: dbmodel.KeyValue{Key: key, Value: false, Type: "bool"},
},
{
kv: model.Int64(key, int64(1499)),
expected: dbmodel.KeyValue{Key: key, Value: "1499", Type: "int64"},
expected: dbmodel.KeyValue{Key: key, Value: int64(1499), Type: "int64"},
},
{
kv: model.Float64(key, float64(15.66)),
expected: dbmodel.KeyValue{Key: key, Value: "15.66", Type: "float64"},
expected: dbmodel.KeyValue{Key: key, Value: 15.66, Type: "float64"},
},
{
kv: model.String(key, longString),
Expand All @@ -134,10 +131,6 @@ func TestConvertKeyValueValue(t *testing.T) {
kv: model.Binary(key, []byte(longString)),
expected: dbmodel.KeyValue{Key: key, Value: hex.EncodeToString([]byte(longString)), Type: "binary"},
},
{
kv: model.KeyValue{VType: 1500, Key: key},
expected: dbmodel.KeyValue{Key: key, Value: "unknown type 1500", Type: "1500"},
},
}

for _, test := range tests {
Expand Down
46 changes: 7 additions & 39 deletions internal/storage/v1/elasticsearch/spanstore/json_span_compare_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,8 @@
package spanstore

import (
"bytes"
"encoding/json"
"sort"
"testing"

"github.com/kr/pretty"
Expand All @@ -16,11 +16,12 @@ import (
"github.com/jaegertracing/jaeger/internal/storage/elasticsearch/dbmodel"
)

func CompareJSONSpans(t *testing.T, expected *dbmodel.Span, actual *dbmodel.Span) {
sortJSONSpan(expected)
sortJSONSpan(actual)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know the reason of testing JSON Spans this way, but this way was leading to a problem that JSON decode treats every number as float64 which was failing the tests. Therefore we shouldn't be decoding and then asserting rather it should be encoding the expected span and then asserting!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think if you're getting differences there may be something wrong with the translation. Despite the name "JSON" the function CompareJSONSpans was comparing two db spans as Go objects. After your change this function is no different than testJSONEncoding

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think so as the problem lies in unmarshaling, the expected span is unmarshalled from the fixture data, when we change the values from string to any in fixtures (that is change "12345" to 12345) then it change it's type to float64 from int64 so in test the expected span expects float64 everywhere where it should have expected int64. Earlier it was not a problem because every value was a string!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fair, but now this function is no different from testJSONEncoding

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we remove it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yurishkuro Can we remove it? Or should we employ some other strategy like after marshalling converting all the int back to int64 from float64? These 2 PRs are blocker for embedding tagDotReplacement so can you please provide an overview over this?


if !assert.Equal(t, expected, actual) {
func CompareJSONSpans(t *testing.T, expected []byte, actual *dbmodel.Span) {
buf := &bytes.Buffer{}
enc := json.NewEncoder(buf)
enc.SetIndent("", " ")
require.NoError(t, enc.Encode(actual))
if !assert.Equal(t, string(expected), buf.String()) {
for _, err := range pretty.Diff(expected, actual) {
t.Log(err)
}
Expand All @@ -29,36 +30,3 @@ func CompareJSONSpans(t *testing.T, expected *dbmodel.Span, actual *dbmodel.Span
t.Logf("Actual trace: %s", string(out))
}
}

func sortJSONSpan(span *dbmodel.Span) {
sortJSONTags(span.Tags)
sortJSONLogs(span.Logs)
sortJSONProcess(span.Process)
}

type JSONTagByKey []dbmodel.KeyValue

func (t JSONTagByKey) Len() int { return len(t) }
func (t JSONTagByKey) Swap(i, j int) { t[i], t[j] = t[j], t[i] }
func (t JSONTagByKey) Less(i, j int) bool { return t[i].Key < t[j].Key }

func sortJSONTags(tags []dbmodel.KeyValue) {
sort.Sort(JSONTagByKey(tags))
}

type JSONLogByTimestamp []dbmodel.Log

func (t JSONLogByTimestamp) Len() int { return len(t) }
func (t JSONLogByTimestamp) Swap(i, j int) { t[i], t[j] = t[j], t[i] }
func (t JSONLogByTimestamp) Less(i, j int) bool { return t[i].Timestamp < t[j].Timestamp }

func sortJSONLogs(logs []dbmodel.Log) {
sort.Sort(JSONLogByTimestamp(logs))
for i := range logs {
sortJSONTags(logs[i].Fields)
}
}

func sortJSONProcess(process dbmodel.Process) {
sortJSONTags(process.Tags)
}
55 changes: 53 additions & 2 deletions internal/storage/v1/elasticsearch/spanstore/to_domain.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -173,13 +173,28 @@

// convertKeyValue expects the Value field to be string, because it only works
// as a reverse transformation after FromDomain() for ElasticSearch model.
func (ToDomain) convertKeyValue(tag *dbmodel.KeyValue) (model.KeyValue, error) {
func (td ToDomain) convertKeyValue(tag *dbmodel.KeyValue) (model.KeyValue, error) {
if tag.Value == nil {
return model.KeyValue{}, fmt.Errorf("invalid nil Value in %v", tag)
}
tagValue, ok := tag.Value.(string)
if !ok {
return model.KeyValue{}, fmt.Errorf("non-string Value of type %t in %v", tag.Value, tag)
switch tag.Type {
case dbmodel.Int64Type, dbmodel.Float64Type:
kv, err := td.fromDBNumber(tag)
if err != nil {
return model.KeyValue{}, err
}
return kv, nil
case dbmodel.BoolType:
if boolVal, ok := tag.Value.(bool); ok {
return model.Bool(tag.Key, boolVal), nil
}
return model.KeyValue{}, invalidValueErr(tag)
// string and binary values should always be of string type
default:
return model.KeyValue{}, invalidValueErr(tag)
}
}
switch tag.Type {
case dbmodel.StringType:
Expand Down Expand Up @@ -212,6 +227,42 @@
return model.KeyValue{}, fmt.Errorf("not a valid ValueType string %s", string(tag.Type))
}

func (ToDomain) fromDBNumber(kv *dbmodel.KeyValue) (model.KeyValue, error) {
if kv.Type == dbmodel.Int64Type {
switch v := kv.Value.(type) {
case int64:
return model.Int64(kv.Key, v), nil
// This case is very much possible as JSON converts every number to float64
case float64:
return model.Int64(kv.Key, int64(v)), nil
case json.Number:
n, err := v.Int64()
if err == nil {
return model.Int64(kv.Key, n), nil
}
default:
return model.KeyValue{}, invalidValueErr(kv)
}
} else if kv.Type == dbmodel.Float64Type {
switch v := kv.Value.(type) {
case float64:
return model.Float64(kv.Key, v), nil
case json.Number:
n, err := v.Float64()
if err == nil {
return model.Float64(kv.Key, n), nil
}
default:
return model.KeyValue{}, invalidValueErr(kv)
}
}
return model.KeyValue{}, fmt.Errorf("not a valid number ValueType %s", string(kv.Type))

Check warning on line 259 in internal/storage/v1/elasticsearch/spanstore/to_domain.go

View check run for this annotation

Codecov / codecov/patch

internal/storage/v1/elasticsearch/spanstore/to_domain.go#L259 

Added line #L259 was not covered by tests
}

func invalidValueErr(kv *dbmodel.KeyValue) error {
return fmt.Errorf("invalid %s type in %+v", string(kv.Type), kv.Value)
}

func (td ToDomain) convertLogs(logs []dbmodel.Log) ([]model.Log, error) {
retMe := make([]model.Log, len(logs))
for i, l := range logs {
Expand Down
118 changes: 110 additions & 8 deletions internal/storage/v1/elasticsearch/spanstore/to_domain_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -163,10 +163,13 @@ func TestFailureBadLogs(t *testing.T) {

func TestRevertKeyValueOfType(t *testing.T) {
tests := []struct {
kv *dbmodel.KeyValue
err string
name string
kv *dbmodel.KeyValue
err string
outKv model.KeyValue
}{
{
name: "not a valid ValueType string",
kv: &dbmodel.KeyValue{
Key: "sneh",
Type: "badType",
Expand All @@ -175,22 +178,121 @@ func TestRevertKeyValueOfType(t *testing.T) {
err: "not a valid ValueType string",
},
{
kv: &dbmodel.KeyValue{},
err: "invalid nil Value",
name: "invalid nil Value",
kv: &dbmodel.KeyValue{},
err: "invalid nil Value",
},
{
name: "right int value",
kv: &dbmodel.KeyValue{
Key: "int-val",
Type: dbmodel.Int64Type,
Value: int64(123),
},
outKv: model.KeyValue{
Key: "int-val",
VInt64: 123,
VType: 2,
},
},
{
name: "right int float value",
kv: &dbmodel.KeyValue{
Key: "int-val",
Type: dbmodel.Int64Type,
Value: float64(123),
},
outKv: model.KeyValue{
Key: "int-val",
VInt64: 123,
VType: 2,
},
},
{
name: "right int json number",
kv: &dbmodel.KeyValue{
Key: "int-val",
Type: dbmodel.Int64Type,
Value: json.Number("123"),
},
outKv: model.KeyValue{
Key: "int-val",
VInt64: 123,
VType: 2,
},
},
{
name: "right float value",
kv: &dbmodel.KeyValue{
Key: "float-val",
Type: dbmodel.Float64Type,
Value: 123.4,
},
outKv: model.KeyValue{
Key: "float-val",
VFloat64: 123.4,
VType: 3,
},
},
{
name: "right float json number",
kv: &dbmodel.KeyValue{
Key: "float-val",
Type: dbmodel.Float64Type,
Value: json.Number("123.4"),
},
outKv: model.KeyValue{
Key: "float-val",
VFloat64: 123.4,
VType: 3,
},
},
{
name: "wrong int64 value",
kv: &dbmodel.KeyValue{
Key: "int-val",
Type: dbmodel.Int64Type,
Value: true,
},
err: "invalid int64 type in true",
},
{
name: "wrong float64 value",
kv: &dbmodel.KeyValue{
Key: "float-val",
Type: dbmodel.Float64Type,
Value: true,
},
err: "invalid float64 type in true",
},
{
name: "wrong bool value",
kv: &dbmodel.KeyValue{
Key: "bool-val",
Type: dbmodel.BoolType,
Value: 1.23,
},
err: "invalid bool type in 1.23",
},
{
name: "wrong string value",
kv: &dbmodel.KeyValue{
Key: "string-val",
Type: dbmodel.StringType,
Value: 123,
},
err: "non-string Value of type",
err: "invalid string type in 123",
},
}
td := ToDomain{}
for _, test := range tests {
t.Run(test.err, func(t *testing.T) {
t.Run(test.name, func(t *testing.T) {
tag := test.kv
_, err := td.convertKeyValue(tag)
assert.ErrorContains(t, err, test.err)
out, err := td.convertKeyValue(tag)
if test.err != "" {
require.ErrorContains(t, err, test.err)
}
assert.Equal(t, test.outKv, out)
})
}
}
Expand Down
Loading