-
Notifications
You must be signed in to change notification settings - Fork 530
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
Extend JSON file reader to handle array of objects with resourceSpans #2254
Conversation
Signed-off-by: adarsh-jaiss <its.adarshjaiss@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add a unit test
Hey @yurishkuro , I made the signigicant changes :
and added these test cases :
and after running Can you please help me out on this? |
please push your changes, I cannot troubleshoot via screenshots |
Signed-off-by: adarsh-jaiss <its.adarshjaiss@gmail.com>
|
Signed-off-by: adarsh-jaiss <its.adarshjaiss@gmail.com>
|
Signed-off-by: Yuri Shkuro <yurishkuro@users.noreply.github.com>
Signed-off-by: Yuri Shkuro <yurishkuro@users.noreply.github.com>
@@ -72,4 +72,20 @@ describe('fileReader.readJsonFile', () => { | |||
const p = readJsonFile({ file }); | |||
return expect(p).rejects.toMatchObject(expect.any(Error)); | |||
}); | |||
|
|||
it('loads JSON data with single resourceSpans array successfully', async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this test is needed, if you can process array of size>1 you can certainly process size=1
|
||
it('loads JSON data with multiple resourceSpans arrays successfully', async () => { | ||
const obj1 = { resourceSpans: [{ id: 1 }, { id: 2 }] }; | ||
const obj2 = { resourceSpans: [{ id: 3 }, { id: 4 }] }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is not a valid OTEL structure, it should be something like resourceSpans > scopeSpans > spans
addressed in #2380 |
Which problem is this PR solving?
Description of the changes
Checklist
jaeger
:make lint test
jaeger-ui
:yarn lint
andyarn test