Skip to content
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.

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

Fixes handling of numeric strings in span tag values #436

Merged
merged 3 commits into from
Aug 11, 2019

Conversation

matus-m
Copy link
Contributor

@matus-m matus-m commented Aug 8, 2019

Signed-off-by: Matus Majchrak matus.majchrak@gmail.com
Fixes the rendering of span tag values that contain numeric strings

Hi this is my first PR to this project so bear with me :)

Resolves #396

Which problem is this PR solving?

As stated in bug #396, span tags containing numeric strings will be converted to actual numbers and rendered in scientific notation if they are large enough.

Short description of the changes

As hinted by @tiffon, this commit limits the json conversion of tag values only to cases, when the value is an actual string and is not a primitive type(i.e. only if it is object or array)

The one open point from my side, is what would be a meaningful test for the rendering of tag value containing real json object(if it is required) as I had some trouble creating an elegant test case that would verify the dangerouslySetInnerHTML contents(right now I verify it by call to html() and then string matching...) and what role should tag type attribute play in the rendering:
e.g.
given span tag payload:

{
  "key": "numberTag",
 "type": "int",
  "value": 123
},
{
  "key": "numberAsString",
 "type": "string",
  "value": "123"
}

Not sure if this is a valid case.
In any case, looking forward to your feedback.

@codecov
Copy link

codecov bot commented Aug 8, 2019

Codecov Report

Merging #436 into master will decrease coverage by 0.04%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #436      +/-   ##
==========================================
- Coverage   90.98%   90.94%   -0.05%     
==========================================
  Files         177      177              
  Lines        4106     4108       +2     
  Branches      991      992       +1     
==========================================
  Hits         3736     3736              
- Misses        327      329       +2     
  Partials       43       43
Impacted Files Coverage Δ
.../TraceTimelineViewer/SpanDetail/KeyValuesTable.tsx 100% <100%> (ø) ⬆️
...eViewer/TimelineHeaderRow/TimelineViewingLayer.tsx 89.83% <0%> (-3.39%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8cfb026...4b61e23. Read the comment docs.

Copy link
Member

@tiffon tiffon left a comment

Choose a reason for hiding this comment

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

This looks great. I think the check could be folded into parseIfJson, i.e. parseIfComplexJson, but this works, too.

@matus-m matus-m force-pushed the fix-numericstring-handling branch 4 times, most recently from 57abda4 to 066ed83 Compare August 9, 2019 12:16
Signed-off-by: Matus Majchrak <matus.majchrak@gmail.com>
Signed-off-by: Matus Majchrak <matus.majchrak@gmail.com>
@matus-m
Copy link
Contributor Author

matus-m commented Aug 9, 2019

alright, could you, please, give it one more try?

Copy link
Member

@tiffon tiffon left a comment

Choose a reason for hiding this comment

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

Thanks for making the changes. A couple of small questions...


function parseIfComplexJson(value: any) {
// if the value is a string representing actual json object or array, then use json-markup
if (typeof value === 'string' && value.match(jsonObjectOrArrayStartRegex)) {
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 this can be jsonObjectOrArrayStartRegex.test(value) to avoid generating match data?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

right

if (typeof value === 'string' && value.match(jsonObjectOrArrayStartRegex)) {
// otherwise just return as is
try {
console.log('parsing JSON');
Copy link
Member

Choose a reason for hiding this comment

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

Is this log statement a left-over?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oops, fixed now

Signed-off-by: Matus Majchrak <matus.majchrak@gmail.com>
Copy link
Member

@tiffon tiffon left a comment

Choose a reason for hiding this comment

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

👍

@tiffon
Copy link
Member

tiffon commented Aug 11, 2019

🎉 Thanks for contributing!! 🎉

@tiffon tiffon merged commit c2453fb into jaegertracing:master Aug 11, 2019
@matus-m
Copy link
Contributor Author

matus-m commented Aug 11, 2019

Pleasure is all mine, well setup project and great feedback.

vvvprabhakar pushed a commit to vvvprabhakar/jaeger-ui that referenced this pull request Jul 5, 2021
Fixes handling of numeric strings in span tag values
Signed-off-by: vvvprabhakar <vvvprabhakar@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

String tags are being interpreted as numbers
2 participants