Support uploading traces in OpenTelemetry format (OTLP JSON) #2145

NavinShrinivas · 2024-01-31T02:15:14Z

Which problem is this PR solving?

Part of Ability to upload OTLP data into UI jaeger#4949

Description of the changes

Adds a redux middleware to make API calls to convert OTLP traces to jaeger traces when uploaded

How was this change tested?

Unit test have been written, but apart form that OTLP traces were fed into the frontend (when compiled using all-in-one) to get correct behabiour

Checklist

I have read https://github.com/jaegertracing/jaeger/blob/master/CONTRIBUTING_GUIDELINES.md
I have signed all commits
I have added unit tests for the new functionality
I have run lint and test steps successfully
- for jaeger: make lint test
- for jaeger-ui: yarn lint and yarn test

packages/jaeger-ui/tsconfig.json

packages/jaeger-ui/src/middlewares/index.js

Signed-off-by: Navin Shrinivas <karupal2002@gmail.com>

packages/jaeger-ui/src/middlewares/fixtures/JaegerTrace.json

Signed-off-by: Navin Shrinivas <karupal2002@gmail.com>

codecov · 2024-02-01T06:26:34Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.59%. Comparing base (aba6c2c) to head (1e33ca7).

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #2145   +/-   ##
=======================================
  Coverage   96.59%   96.59%           
=======================================
  Files         254      254           
  Lines        7629     7639   +10     
  Branches     1988     1990    +2     
=======================================
+ Hits         7369     7379   +10     
  Misses        260      260

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

NavinShrinivas · 2024-02-02T08:52:27Z

Hey, are you waiting for commits to meet the test requirement? I'd like to more this pr forward with necessary changes

yurishkuro · 2024-02-05T01:21:12Z

please make sure the CI is green.

Signed-off-by: Navin Shrinivas <karupal2002@gmail.com>

yurishkuro · 2024-02-11T21:10:56Z

I found one issue. When I try to upload OTLP-formatted file, I am getting an exception in the UI code:

in this function

function loadJsonDone(state, { payload }) {
  const processed = payload.data.map(transformTraceData);

The payload here is still OTLP-formatted, so it has no .data and .map throws. I don't fully understand how these "actions" work, but is it perhaps possible to ensure that your new transformer action is called before the execution reaches into loadJsonDone?

yurishkuro · 2024-02-11T21:11:57Z

And on a related question - is it possible to ensure that we have a unit test that catches this?

#2145 (comment)

Signed-off-by: Navin Shrinivas <karupal2002@gmail.com>

yurishkuro · 2024-02-13T06:27:57Z

packages/jaeger-ui/src/middlewares/index.js

@@ -70,7 +70,7 @@ export const loadJsonTracesMiddleware = store => next => action => {
  } else {
    return next(action);
  }
-  return next(action); // Consistent returns
+  return undefined;


why is this the right behavior?

I assume calling the next action is a pattern that needs to be maintained, it looks like a dangerous shortcut when you change it just to work around another problem. The right fix is to control the order of these middlewares, so that OTLP translator is invoked before the rest of the handlers.

…5155) ## Which problem is this PR solving? * Solves #4949 * In combination with jaegertracing/jaeger-ui#2145 ## Description of the changes - Incorporates changes from previous draft PR review - Adds middleware in frontend as well ## How was this change tested? - Unit tests and supplying valid and invalid OTLP traces using insomnia to test the API ## Checklist - [x] I have read https://github.com/jaegertracing/jaeger/blob/master/CONTRIBUTING_GUIDELINES.md - [x] I have signed all commits - [x] I have added unit tests for the new functionality - [x] I have run lint and test steps successfully - for `jaeger`: `make lint test` - for `jaeger-ui`: `yarn lint` and `yarn test` --------- Signed-off-by: Navin Shrinivas <karupal2002@gmail.com> Signed-off-by: Yuri Shkuro <github@ysh.us> Co-authored-by: Yuri Shkuro <yurishkuro@users.noreply.github.com> Co-authored-by: Yuri Shkuro <github@ysh.us>

Signed-off-by: Navin Shrinivas <karupal2002@gmail.com>

packages/jaeger-ui/src/utils/readJsonFile.tsx

yurishkuro

I tried a few other ways, but there's a complication with JaegerAPI returning a future while all other code paths are synchronous. Will merge as is.

yurishkuro · 2024-03-04T03:45:17Z

thanks!

NavinShrinivas · 2024-03-04T03:46:50Z

thanks!

It was fun working on this issues. Learnt a lot :) , any other issue that i can work on that are beginner friendly?

yurishkuro · 2024-03-04T04:18:28Z

https://github.com/search?q=org%3Ajaegertracing+label%3A%22good+first+issue%22&type=issues&state=open

NavinShrinivas mentioned this pull request Jan 31, 2024

Support uploading traces to UI in OpenTelemetry format (OTLP JSON) jaegertracing/jaeger#5155

Merged

4 tasks

yurishkuro reviewed Jan 31, 2024

View reviewed changes

packages/jaeger-ui/tsconfig.json Outdated Show resolved Hide resolved

yurishkuro reviewed Jan 31, 2024

View reviewed changes

packages/jaeger-ui/src/middlewares/index.js Outdated Show resolved Hide resolved

yurishkuro reviewed Jan 31, 2024

View reviewed changes

packages/jaeger-ui/src/middlewares/index.js Outdated Show resolved Hide resolved

yurishkuro reviewed Jan 31, 2024

View reviewed changes

packages/jaeger-ui/src/middlewares/index.js Outdated Show resolved Hide resolved

NavinShrinivas force-pushed the uploading_otlp_traces branch from a80f061 to ced668d Compare January 31, 2024 03:32

Add middleware to accept OTLP traces when uploaded

b99756d

Signed-off-by: Navin Shrinivas <karupal2002@gmail.com>

NavinShrinivas force-pushed the uploading_otlp_traces branch from ced668d to b99756d Compare January 31, 2024 03:33

yurishkuro reviewed Feb 1, 2024

View reviewed changes

packages/jaeger-ui/src/middlewares/fixtures/JaegerTrace.json Outdated Show resolved Hide resolved

Adding unit test middleware

ac50454

Signed-off-by: Navin Shrinivas <karupal2002@gmail.com>

NavinShrinivas force-pushed the uploading_otlp_traces branch from 39cceaf to ac50454 Compare February 1, 2024 03:10

yurishkuro added the changelog:new-feature Change that should be called out as new feature in CHANGELOG label Feb 5, 2024

yurishkuro changed the title ~~Add middleware to accept OTLP traces when uploaded~~ Support uploading traces in OpenTelemetry format (OTLP JSON) Feb 5, 2024

NavinShrinivas and others added 2 commits February 7, 2024 11:02

Adding more tests and complying to CI/CD pipeline

aaced54

Signed-off-by: Navin Shrinivas <karupal2002@gmail.com>

Merge branch 'main' into uploading_otlp_traces

9599ede

yurishkuro marked this pull request as ready for review February 11, 2024 21:05

yurishkuro requested a review from a team as a code owner February 11, 2024 21:05

yurishkuro requested review from joe-elliott and removed request for a team February 11, 2024 21:05

yurishkuro previously approved these changes Feb 11, 2024

View reviewed changes

Fixing error

f3827b0

Signed-off-by: Navin Shrinivas <karupal2002@gmail.com>

yurishkuro reviewed Feb 13, 2024

View reviewed changes

Removing middleware and including API call as part of dispatch func

eb689c2

Signed-off-by: Navin Shrinivas <karupal2002@gmail.com>

Adding same test files as backend

544bc25

Signed-off-by: Navin Shrinivas <karupal2002@gmail.com>

NavinShrinivas requested a review from yurishkuro February 15, 2024 12:02

Removing few useless imports

2bd3a2f

Signed-off-by: Navin Shrinivas <karupal2002@gmail.com>

NavinShrinivas force-pushed the uploading_otlp_traces branch from f40568f to 2bd3a2f Compare February 15, 2024 12:08

yurishkuro reviewed Feb 15, 2024

View reviewed changes

packages/jaeger-ui/src/utils/readJsonFile.tsx Show resolved Hide resolved

NavinShrinivas requested a review from yurishkuro February 19, 2024 08:08

yurishkuro reviewed Feb 19, 2024

View reviewed changes

packages/jaeger-ui/src/utils/readJsonFile.tsx Show resolved Hide resolved

yurishkuro approved these changes Mar 4, 2024

View reviewed changes

Merge branch 'main' into uploading_otlp_traces

1e33ca7

yurishkuro merged commit d8bf9b9 into jaegertracing:main Mar 4, 2024
8 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support uploading traces in OpenTelemetry format (OTLP JSON) #2145

Support uploading traces in OpenTelemetry format (OTLP JSON) #2145

NavinShrinivas commented Jan 31, 2024 •

edited

Loading

codecov bot commented Feb 1, 2024 •

edited

Loading

NavinShrinivas commented Feb 2, 2024

yurishkuro commented Feb 5, 2024

yurishkuro commented Feb 11, 2024

yurishkuro commented Feb 11, 2024

yurishkuro Feb 13, 2024

yurishkuro Feb 14, 2024

yurishkuro left a comment

yurishkuro commented Mar 4, 2024

NavinShrinivas commented Mar 4, 2024 •

edited

Loading

yurishkuro commented Mar 4, 2024

Support uploading traces in OpenTelemetry format (OTLP JSON) #2145

Support uploading traces in OpenTelemetry format (OTLP JSON) #2145

Conversation

NavinShrinivas commented Jan 31, 2024 • edited Loading

Which problem is this PR solving?

Description of the changes

How was this change tested?

Checklist

codecov bot commented Feb 1, 2024 • edited Loading

Codecov Report

NavinShrinivas commented Feb 2, 2024

yurishkuro commented Feb 5, 2024

yurishkuro commented Feb 11, 2024

yurishkuro commented Feb 11, 2024

yurishkuro Feb 13, 2024

Choose a reason for hiding this comment

yurishkuro Feb 14, 2024

Choose a reason for hiding this comment

yurishkuro left a comment

Choose a reason for hiding this comment

yurishkuro commented Mar 4, 2024

NavinShrinivas commented Mar 4, 2024 • edited Loading

yurishkuro commented Mar 4, 2024

NavinShrinivas commented Jan 31, 2024 •

edited

Loading

codecov bot commented Feb 1, 2024 •

edited

Loading

NavinShrinivas commented Mar 4, 2024 •

edited

Loading