[Bug]: span tags of type int64 may lose precision #3958 #4023

shubbham1215 · 2022-11-07T19:46:59Z

Avoid value precision loss

Signed-off-by: Shubham Sawaiker [email protected]

Which problem is this PR solving?

Resolves [Bug]: span tags of type int64 may lose precision #3958

Short description of the changes

avoid display value precision loss in case of value overflow Number.MAX_VALUE

shubbham1215 · 2022-11-07T19:56:31Z

@yurishkuro Could you please check this draft pull request? I suppose the types in domain_01.json and ui_01.json are to be different in case of large integer values.

yurishkuro · 2022-11-07T20:48:32Z

model/converter/json/from_domain.go

@@ -122,6 +128,10 @@ func (fd fromDomain) convertKeyValues(keyValues model.KeyValues) []json.KeyValue
 			value = kv.Bool()
 		case model.Int64Type:
 			value = kv.Int64()
+			if kv.Int64() > js_limit_max || kv.Int64() < js_limit_min {
+				kv.VType = 0


this needs to go into temp variable above the switch, must not change (in place) the input data

I could not get why this change is required. If we move this if
if kv.Int64() > jsMaxSafeInteger || kv.Int64() < jsMinSafeInteger {
kv.VType = model.StringType
value = fmt.Sprintf("%d", value)
}
before switch, the switch case model.StringType will be executed and the previous value will be overwriiten.

You should not be changing the type at all, just the value to be a string.

yurishkuro · 2022-11-07T20:53:52Z

model/converter/json/from_domain.go

+	js_limit_max = 9007199254740991
+	js_limit_min = -9007199254740991


Suggested change

js_limit_max = 9007199254740991

js_limit_min = -9007199254740991

jsMaxSafeInteger = int64(1)<<53 - 1

jsMinSafeInteger = -jsMaxSafeInteger

yurishkuro · 2022-11-07T20:55:56Z

model/converter/json/fixtures/ui_01.json

+        {
+          "key":"javascript_limit",
+          "vType":"INT64",
+          "vInt64":9223372036854775222


I expect this to be string, otherwise it will lose precision upon parsing in JS

Suggested change

"vInt64":9223372036854775222

"vInt64":"9223372036854775222"

const z = Number.parseInt("9007199254740993"); console.log(z); // expected output: 9007199254740992

Yes it was shown as a string in ui_01-actual.json . There is a type mismatch between domain_01.json (int64) and ui_01-actual.json (string) and hence the test case TestFromDomain was failing

I am not talking about vType field, it should indeed remain INT64, but the value of vInt64 field needs to be a string, not a raw number, because the raw number is what's causing the precision loss.

yurishkuro · 2022-11-07T21:08:25Z

please make sure commits are signed (git commit -s), see CONTRIBUTING.md

yurishkuro

You still have some commits unsigned. I recommend squashing them into one commit that you can sign.

yurishkuro · 2022-11-08T18:08:41Z

model/converter/json/fixtures/ui_01.json

+        {
+          "key":"javascript_limit",
+          "vType":"INT64",
+          "vInt64":9223372036854775222


I am not talking about vType field, it should indeed remain INT64, but the value of vInt64 field needs to be a string, not a raw number, because the raw number is what's causing the precision loss.

yurishkuro · 2022-11-08T18:09:12Z

model/converter/json/from_domain.go

@@ -122,6 +128,10 @@ func (fd fromDomain) convertKeyValues(keyValues model.KeyValues) []json.KeyValue
 			value = kv.Bool()
 		case model.Int64Type:
 			value = kv.Int64()
+			if kv.Int64() > js_limit_max || kv.Int64() < js_limit_min {
+				kv.VType = 0


You should not be changing the type at all, just the value to be a string.

yurishkuro · 2022-11-08T18:09:53Z

model/converter/json/from_domain.go

@@ -122,6 +128,10 @@ func (fd fromDomain) convertKeyValues(keyValues model.KeyValues) []json.KeyValue
 			value = kv.Bool()
 		case model.Int64Type:
 			value = kv.Int64()
+			if kv.Int64() > jsMaxSafeInteger || kv.Int64() < jsMinSafeInteger {
+				kv.VType = 0


Suggested change

kv.VType = 0

I have done the required changes. Could you please review?

Signed-off-by: Shubham Sawaiker <[email protected]>

[Bug]: span tags of type int64 may lose precision jaegertracing#3958 Signed-off-by: Shubham Sawaiker <[email protected]>

Bumps [github.com/spf13/viper](https://github.com/spf13/viper) from 1.13.0 to 1.14.0. - [Release notes](https://github.com/spf13/viper/releases) - [Commits](spf13/viper@v1.13.0...v1.14.0) --- updated-dependencies: - dependency-name: github.com/spf13/viper dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]> Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Signed-off-by: Shubham Sawaiker <[email protected]>

…tracing#4026) Bumps [github.com/dgraph-io/badger/v3](https://github.com/dgraph-io/badger) from 3.2103.3 to 3.2103.4. - [Release notes](https://github.com/dgraph-io/badger/releases) - [Changelog](https://github.com/dgraph-io/badger/blob/main/CHANGELOG.md) - [Commits](dgraph-io/badger@v3.2103.3...v3.2103.4) --- updated-dependencies: - dependency-name: github.com/dgraph-io/badger/v3 dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <[email protected]> Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Signed-off-by: Shubham Sawaiker <[email protected]>

Bumps [github/codeql-action](https://github.com/github/codeql-action) from 2.1.29 to 2.1.31. - [Release notes](https://github.com/github/codeql-action/releases) - [Changelog](https://github.com/github/codeql-action/blob/main/CHANGELOG.md) - [Commits](github/codeql-action@ec3cf9c...c3b6fce) --- updated-dependencies: - dependency-name: github/codeql-action dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <[email protected]> Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Signed-off-by: Shubham Sawaiker <[email protected]>

Bumps [anchore/sbom-action](https://github.com/anchore/sbom-action) from 0.13.0 to 0.13.1. - [Release notes](https://github.com/anchore/sbom-action/releases) - [Commits](anchore/sbom-action@b7e8507...06e1094) --- updated-dependencies: - dependency-name: anchore/sbom-action dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <[email protected]> Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Signed-off-by: Shubham Sawaiker <[email protected]>

Signed-off-by: Shubham Sawaiker <[email protected]>

[Bug]: span tags of type int64 may lose precision

yurishkuro · 2022-11-08T20:27:03Z

model/converter/json/from_domain.go

 		switch kv.VType {
 		case model.StringType:
 			value = kv.VStr
 		case model.BoolType:
 			value = kv.Bool()
 		case model.Int64Type:
 			value = kv.Int64()
+			if jsflag {


Suggested change

if jsflag {

if kv.Int64() > jsMaxSafeInteger || kv.Int64() < jsMinSafeInteger {

yurishkuro · 2022-11-08T20:27:15Z

model/converter/json/from_domain.go

+		var jsflag bool = false
+		if kv.Int64() > jsMaxSafeInteger || kv.Int64() < jsMinSafeInteger {
+			jsflag = true
+		}


Suggested change

var jsflag bool = false

if kv.Int64() > jsMaxSafeInteger || kv.Int64() < jsMinSafeInteger {

jsflag = true

}

codecov · 2022-11-08T20:35:25Z

Codecov Report

Base: 97.18% // Head: 97.16% // Decreases project coverage by -0.02% ⚠️

Coverage data is based on head (1a845ba) compared to base (84c0abc).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4023      +/-   ##
==========================================
- Coverage   97.18%   97.16%   -0.03%     
==========================================
  Files         295      295              
  Lines       17390    17393       +3     
==========================================
- Hits        16901    16900       -1     
- Misses        394      397       +3     
- Partials       95       96       +1

Flag	Coverage Δ
unittests	`97.16% <100.00%> (-0.03%)`	⬇️

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

Impacted Files	Coverage Δ
model/converter/json/from_domain.go	`100.00% <100.00%> (ø)`
plugin/storage/badger/spanstore/reader.go	`95.49% <0.00%> (-0.72%)`	⬇️
plugin/storage/integration/integration.go	`75.95% <0.00%> (-0.39%)`	⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

[Bug]: span tags of type int64 may lose precision

yurishkuro · 2022-11-08T20:58:11Z

Thanks!

shubbham1215 · 2022-11-09T09:34:11Z

Do I need to make more changes? I saw that some tests related to docker are failing.

yurishkuro · 2022-11-09T15:38:59Z

probably spurious failures. I have the auto-merge enabled, will keep an eye on the CI

yurishkuro · 2022-11-09T16:58:26Z

ah, I just realized the issue - your PR is opened from the main branch of your fork, this is why the CI is failing. Please create another branch and open a new PR.

yurishkuro · 2022-11-09T20:36:46Z

replaced by #4034

### Avoid value precision loss Signed-off-by: Shubham Sawaiker [[email protected]](mailto:[email protected]) ### Which problem is this PR solving? - Resolves: #3958 ### Short description of the changes - avoid display value precision loss in case of value overflow Number.MAX_VALUE Previous PR was made from main branch because of which CI jobs were failing. First PR link : #4023 Signed-off-by: Shubham Sawaiker <[email protected]> Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

shubbham1215 requested a review from a team as a code owner November 7, 2022 19:46

shubbham1215 requested a review from joe-elliott November 7, 2022 19:47

shubbham1215 marked this pull request as draft November 7, 2022 19:50

yurishkuro reviewed Nov 7, 2022

View reviewed changes

yurishkuro reviewed Nov 8, 2022

View reviewed changes

shubbham1215 and others added 7 commits November 9, 2022 01:02

[Bug]: span tags of type int64 may lose precision jaegertracing#3958

7567490

Signed-off-by: Shubham Sawaiker <[email protected]>

Signed-off-by: Shubham Sawaiker <[email protected]>

ae3f76b

[Bug]: span tags of type int64 may lose precision jaegertracing#3958 Signed-off-by: Shubham Sawaiker <[email protected]>

[Bug]: span tags of type int64 may lose precision

b1d5a1c

Signed-off-by: Shubham Sawaiker <[email protected]>

shubbham1215 force-pushed the main branch from 04ab597 to b1d5a1c Compare November 8, 2022 19:33

shubbham1215 and others added 3 commits November 9, 2022 01:06

Merge branch 'jaegertracing:main' into main

d02aa0a

Signed-off-by: Shubham Sawaiker <[email protected]>

e20eddb

[Bug]: span tags of type int64 may lose precision

Merge branch 'main' of https://github.com/shubbham1215/jaeger into main

c43bc5d

shubbham1215 marked this pull request as ready for review November 8, 2022 20:00

yurishkuro reviewed Nov 8, 2022

View reviewed changes

Signed-off-by: Shubham Sawaiker <[email protected]>

a95117a

[Bug]: span tags of type int64 may lose precision

yurishkuro approved these changes Nov 8, 2022

View reviewed changes

yurishkuro enabled auto-merge (squash) November 8, 2022 20:58

Merge branch 'jaegertracing:main' into main

1a845ba

shubbham1215 mentioned this pull request Nov 9, 2022

[Bug]: span tags of type int64 may lose precision #3958 PR No 2 #4034

Merged

yurishkuro closed this Nov 9, 2022

auto-merge was automatically disabled November 9, 2022 20:36
Pull request was closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Bug]: span tags of type int64 may lose precision #3958 #4023

[Bug]: span tags of type int64 may lose precision #3958 #4023

shubbham1215 commented Nov 7, 2022 •

edited

Loading

shubbham1215 commented Nov 7, 2022

yurishkuro Nov 7, 2022

shubbham1215 Nov 8, 2022

yurishkuro Nov 8, 2022

yurishkuro Nov 7, 2022

yurishkuro Nov 7, 2022

yurishkuro Nov 7, 2022

shubbham1215 Nov 8, 2022

yurishkuro Nov 8, 2022

yurishkuro commented Nov 7, 2022

yurishkuro left a comment

yurishkuro Nov 8, 2022

yurishkuro Nov 8, 2022

yurishkuro Nov 8, 2022

shubbham1215 Nov 8, 2022

yurishkuro Nov 8, 2022

yurishkuro Nov 8, 2022

shubbham1215 Nov 8, 2022

codecov bot commented Nov 8, 2022 •

edited

Loading

yurishkuro commented Nov 8, 2022

shubbham1215 commented Nov 9, 2022

yurishkuro commented Nov 9, 2022

yurishkuro commented Nov 9, 2022

yurishkuro commented Nov 9, 2022

		js_limit_max = 9007199254740991
		js_limit_min = -9007199254740991

	if jsflag {
	if kv.Int64() > jsMaxSafeInteger \|\| kv.Int64() < jsMinSafeInteger {

[Bug]: span tags of type int64 may lose precision #3958 #4023

[Bug]: span tags of type int64 may lose precision #3958 #4023

Conversation

shubbham1215 commented Nov 7, 2022 • edited Loading

Which problem is this PR solving?

shubbham1215 commented Nov 7, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yurishkuro commented Nov 7, 2022

yurishkuro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Nov 8, 2022 • edited Loading

Codecov Report

yurishkuro commented Nov 8, 2022

shubbham1215 commented Nov 9, 2022

yurishkuro commented Nov 9, 2022

yurishkuro commented Nov 9, 2022

yurishkuro commented Nov 9, 2022

shubbham1215 commented Nov 7, 2022 •

edited

Loading

codecov bot commented Nov 8, 2022 •

edited

Loading