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

Extend JSON file reader to handle array of objects with resourceSpans #2254

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

Adarsh-jaiss
Copy link

Which problem is this PR solving?

Description of the changes

  • This commit updates the JSON file reader function to handle an array of JSON objects containing 'resourceSpans' arrays. It now merges these arrays into a single 'resourceSpans' array before processing, ensuring compatibility with both single JSON objects and arrays of objects.

Checklist

Signed-off-by: adarsh-jaiss <its.adarshjaiss@gmail.com>
@Adarsh-jaiss Adarsh-jaiss requested a review from a team as a code owner April 13, 2024 11:46
@Adarsh-jaiss Adarsh-jaiss requested review from joe-elliott and removed request for a team April 13, 2024 11:46
Copy link
Member

@yurishkuro yurishkuro left a 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

packages/jaeger-ui/src/utils/readJsonFile.tsx Outdated Show resolved Hide resolved
@Adarsh-jaiss
Copy link
Author

Hey @yurishkuro , I made the signigicant changes :

try {
        let traceObj = JSON.parse(reader.result);
        if (Array.isArray(traceObj) && traceObj.every(obj => 'resourceSpans' in obj)) {
          const mergedResourceSpans = traceObj.reduce((acc, obj) => {
            acc.push(...obj.resourceSpans);
            return acc;
          }, []);

          traceObj = { resourceSpans: mergedResourceSpans };
        }
        if ('resourceSpans' in traceObj) {
          JaegerAPI.transformOTLP(traceObj)
            .then((result: string) => {
              resolve(result);
            })
            .catch(() => {
              reject(new Error(`Error converting traces to OTLP`));
            });
        }
      } catch (error: unknown) {
        reject(new Error(`Error parsing JSON: ${(error as Error).message}`));
      }
    };

and added these test cases :

  it('loads JSON data with single resourceSpans array successfully', async () => {
    const obj = { resourceSpans: [{ id: 1 }, { id: 2 }, { id: 3 }] };
    const file = new File([JSON.stringify(obj)], 'singleResourceSpans.json');
    const result = await readJsonFile({ file });
    expect(result).toEqual(JSON.stringify(obj));
  });

  it('loads JSON data with multiple resourceSpans arrays successfully', async () => {
    const obj1 = { resourceSpans: [{ id: 1 }, { id: 2 }] };
    const obj2 = { resourceSpans: [{ id: 3 }, { id: 4 }] };
    const file = new File([JSON.stringify([obj1, obj2])], 'multipleResourceSpans.json');
    const expectedResult = { resourceSpans: [...obj1.resourceSpans, ...obj2.resourceSpans] };
    const result = await readJsonFile({ file });
    expect(result).toEqual(JSON.stringify(expectedResult));
  });

and after running npm test, I'm getting this result :
image

Can you please help me out on this?

@yurishkuro
Copy link
Member

please push your changes, I cannot troubleshoot via screenshots

Signed-off-by: adarsh-jaiss <its.adarshjaiss@gmail.com>
@Adarsh-jaiss
Copy link
Author

  • Hey @yurishkuro , I have pushed the code, kindly look into it!

@yurishkuro yurishkuro added the changelog:bugfix-or-minor-feature 🐞 Bug fixes, Minor Improvements label Apr 21, 2024
Adarsh-jaiss and others added 2 commits April 22, 2024 11:57
Signed-off-by: adarsh-jaiss <its.adarshjaiss@gmail.com>
@yurishkuro
Copy link
Member

yarn lint and yarn test are failing

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 () => {
Copy link
Member

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 }] };
Copy link
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog:bugfix-or-minor-feature 🐞 Bug fixes, Minor Improvements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: Importing OTLP "File Exporter" JSON fails
2 participants