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

Ability to upload OTLP data into UI #4949

Closed
charconstpointer opened this issue Nov 14, 2023 · 26 comments
Closed

Ability to upload OTLP data into UI #4949

charconstpointer opened this issue Nov 14, 2023 · 26 comments
Labels
changelog:new-feature Change that should be called out as new feature in CHANGELOG help wanted Features that maintainers are willing to accept but do not have cycles to implement

Comments

@charconstpointer
Copy link

charconstpointer commented Nov 14, 2023

What happened?

I'm running OTel Collector with one of the exporters exporting traces to a file via https://opentelemetry.io/docs/specs/otel/protocol/file-exporter/

Jaeger is running with COLLECTOR_OTLP_ENABLED: true

When trying to import this data to a Jaeger UI via
Screenshot 2023-11-14 at 12 27 43

I'm getting

There was an error querying for traces:
Error parsing JSON: Unexpected non-whitespace character after JSON at position 9465 (line 2 column 1)

Steps to reproduce

  1. Export traces via file exporter(otel collector) to a file
  2. Try to import them manually via Jaeger UI

Expected behavior

To have my parses interpreted correctly as it happens with any other OTLP ingestion

Relevant log output

There was an error querying for traces:
Error parsing JSON: Unexpected non-whitespace character after JSON at position 9465 (line 2 column 1)

Screenshot

No response

Additional context

No response

Jaeger backend version

1.50

SDK

  • jaegertracing/all-in-one:1.50
  • otel col 0.86

Pipeline

OTel SDK -> OTel Collector -> file -> Jaeger manual import via UI

Stogage backend

a file

Operating system

Linux

Deployment model

k8s

Deployment configs

COLLECTOR_OTLP_ENABLED:  true
receivers:
  otlp:
    protocols:
      grpc:

exporters:
  file:
    path: "/tmp/otel-output.json"

processors:

service:
  pipelines:
    traces:
      receivers: [otlp]
      processors: []
      exporters: [file]
@yurishkuro yurishkuro changed the title [Bug]: Importing OTLP data via file upload does not work Ability to upload OTLP data into UI Nov 14, 2023
@yurishkuro yurishkuro added changelog:new-feature Change that should be called out as new feature in CHANGELOG and removed bug labels Nov 14, 2023
@yurishkuro
Copy link
Member

OTLP upload is not supported in the UI, but it's a nice feature which isn't too hard to implement. The way I can see it working is:

  • UI detects that the uploaded file format is OTEL, not Jaeger JSON
  • UI calls an end point in the query service to convert the trace from OTLP into Jaeger format

@yurishkuro yurishkuro added the help wanted Features that maintainers are willing to accept but do not have cycles to implement label Nov 14, 2023
@killua1zoldyck
Copy link

If no one else is working on this, I can take this up.

@sirine707
Copy link

No one assigned, should i start working on it ?

@yurishkuro
Copy link
Member

go for it

@NavinShrinivas
Copy link
Contributor

From what I can understand this task is majorly in the frontend, is there an issue this links to the Jaeger-ui repo?

@NavinShrinivas
Copy link
Contributor

NavinShrinivas commented Jan 9, 2024

also, @charconstpointer Do you by any chance have the otel collector exported file with you? Will help me replicate and implement this feature.

@charconstpointer
Copy link
Author

also, @charconstpointer Do you by any chance have the otel collector exported file with you? Will help me replicate and implement this feature.

No, I have nothing that I'm allowed to share with a wider audience.
I could maybe prep something by the end of the week, e.g. docker compose file that would simulate some traffic end export such file, would that be helpful for you?

@yurishkuro
Copy link
Member

From what I can understand this task is majorly in the frontend, is there an issue this links to the Jaeger-ui repo?

  1. I disagree about the "majority", I think defining a conversion service is more work.

  2. when a feature spans both be and fe we prefer to keep the tracking issue in the backend repo.

@NavinShrinivas
Copy link
Contributor

NavinShrinivas commented Jan 10, 2024

  1. I disagree about the "majority", I think defining a conversion service is more work.

There already exists endpoints such as '4317' in Jager Backend that can accept OTLP traces right? There would not be a need for conversion if the frontend can figure out what kind of trace it is and send it to the right port in the backend. Or do we want to handle this in the backend instead by calling the right functions to convert Jaeger traces to OTLP ones?

@yurishkuro
Copy link
Member

Endpoints in the backend are stateful, they accept traces and store them in the storage backend. This is not what's needed here - the UI needs to call a stateless conversion service that does not store the trace, but returns it in a different format.

@NavinShrinivas
Copy link
Contributor

Ah! That makes sense, I think I have a grip on the issue now and I went through the code a fair bit and found otlp2jaeger package that I think will help me here. I have some doubts regarding the codebase structure. I assume this new endpoint will go in the query module that is part of cmd or will it be part of the collector module? I'm new to this codebase and I couldn't find any indication of this in the CONTRIBUTING.md

And just to mention, I'd like to work on this issue :)

@yurishkuro
Copy link
Member

yes, it would be part of cmd/query. I'd say it can be part of the /api that the UI calls, like /api/transform

@NavinShrinivas
Copy link
Contributor

NavinShrinivas commented Jan 10, 2024

Do you have anything in mind as to how to send the file content across to the backend at least concerning the http endpoint?

@yurishkuro
Copy link
Member

Http post. UI already does it for Archive button.

@NavinShrinivas
Copy link
Contributor

NavinShrinivas commented Jan 11, 2024

This is the format changes in thinking of :

request-> JSON (OTLP) -> PROTOBUF (OTLP) --[using otlp2jaeger package]-> PROTOBUF (jaeger) -> JSON(jaeger) -> response

So I've been trying to convert a protobuf encoded as JSON coming in through HTTP. While doing so I read the OTLP specification in which it mentioned that the OTLP/JSON deviates wrt to TraceId and SpanId where those are not base64 encoded (the value least, and is hex-encoded).

I found this code to handle this sort of unmarshalling in the protobuf :
file path : jaeger/model/v2/traceid.go line 80 (which is used in otel tracev1 protobuf definition)

// UnmarshalJSON inflates trace id from hex string, possibly enclosed in quotes.
// Called by Protobuf JSON deserialization.
func (tid *TraceID) UnmarshalJSON(data []byte) error {
	// in base64 encoding a 16-byte array is padded to 18 bytes
	buf := [traceIDSize + 2]byte{}
	if err := unmarshalJSON(buf[:], data); err != nil {
		return err
	}
	*tid = [traceIDSize]byte{}
	copy(tid[:], buf[:traceIDSize])
	return nil
}

But as per my calculations, this is what I find :

16-byte array will take up 32 characters of hex (as seen in JSON). These 32 characters will take up 192 bits (as base 64 is represented using 6 bits) which is 24 byte array and not 18. Please help me out if I'm mistaken here.

@yurishkuro
Copy link
Member

You need to use code from otel-collector to both unmarshal JSON and to conver it into Jaeger model/*. Then use Jaeger model/converter/json/

@NavinShrinivas
Copy link
Contributor

NavinShrinivas commented Jan 15, 2024

So I've gotten the traces in []model.Batch format, I'm confused as to what the front end can handle, an []mode. Span or an []model.Trace (I assume the input file can have many traces and hence we cannot just simply merge all of them into a single array of span).

Converting a model.batch to model.Trace is causing a bit of an issue as Process type doesn't convert to ProcessMap type if we try json marshalling and unmarshalling which eventually causes issues on the json.fromDomain() function. Would it be okay to define a function on model.Batch name ToTrace that handles these ProcessMaps or is there an alternative?

@yurishkuro
Copy link
Member

If you have an HTTP endpoint that can accept OTLP, then you can use OTEL's code to convert it to Jaeger model, and then use this method in query's http_handler

func (aH *APIHandler) convertModelToUI

@Pushkarm029
Copy link
Member

@NavinShrinivas, are you still working on this one?

@NavinShrinivas
Copy link
Contributor

@NavinShrinivas, are you still working on this one?

Yes, i am still actively trying to solve this issue

@NavinShrinivas
Copy link
Contributor

NavinShrinivas commented Jan 23, 2024

So @yurishkuro I have some updates on this issue,

Currently this is the state on the backend :
recvieing OTLP trace (Recives it as GRPC encoded byte array) -> converts it to ptrace (otel-col type) -> converts to []model.Batch (using ProtoFromTraces from the otel-col-contrib pkg) -> converts it to []model.Trace (using a function I wrote and I have questions regarding) -> converts to []ui.Trace (using convertModelToUI) -> final output of jeager traces in json format

The function I wrote to convert a model.batch to model.Trace looks like this :

func (m *Batch)ConvertToTraces()(*Trace){
   ret_trace := Trace{}
   ret_trace.Spans = m.Spans
   for _,v := range ret_trace.Spans{
      v.Process = m.Process
   }
   ret_trace.ProcessMap = append(ret_trace.ProcessMap, Trace_ProcessMapping{
      ProcessID: m.Process.ServiceName,
      Process: *m.Process,
   })
   return &ret_trace

}

This fixed the issues I was having regarding fromDomain function (Used by convertToUI) giving nill pointer derf on span.Process, I want to know if this is alright and valid or if there is some other way to handle this?

@yurishkuro
Copy link
Member

Please create a draft PR, it's not efficient to discuss small snippets.

NavinShrinivas added a commit to NavinShrinivas/jaeger that referenced this issue Jan 23, 2024
Signed-off-by: Navin Shrinivas <karupal2002@gmail.com>

partial work for issue jaegertracing#4949
@NavinShrinivas
Copy link
Contributor

Hey also, for the frontend, is there any particular way you have in mind for checking if the uploaded is otlp or jaeger. I was thinking of checking the existence resourceSpans key in the payload and making the API call, if anything goes wrong after that we throw the error instead of validating errors before transforming.

@yurishkuro
Copy link
Member

I was thinking of checking the existence resourceSpans key in the payload

sounds reasonable

@NavinShrinivas
Copy link
Contributor

So I have been working on the front end, took me a while to understand the code base, I am adding a middleware to trigger API calls when needed to keep redux reducers "side effect" free. I'll make a draft PR soon.

yurishkuro added a commit that referenced this issue Feb 14, 2024
…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>
yurishkuro added a commit to jaegertracing/jaeger-ui that referenced this issue Mar 4, 2024
## Which problem is this PR solving?
- Part of jaegertracing/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
- [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>
Co-authored-by: Yuri Shkuro <yurishkuro@users.noreply.github.com>
@yurishkuro
Copy link
Member

resolved

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog:new-feature Change that should be called out as new feature in CHANGELOG help wanted Features that maintainers are willing to accept but do not have cycles to implement
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants