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

Add endpoints to allow uploading OTLP traces #5138

Conversation

NavinShrinivas
Copy link
Contributor

@NavinShrinivas NavinShrinivas commented Jan 23, 2024

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

partial work for issue #4949

Which problem is this PR solving?

Description of the changes

  • Adds an endpoint that converts OTLP traces to jaeger traces

How was this change tested?

  • Tested using example traces placed in idl/otel-proto/examples/traces.json

The commit in the PR is rather rough, looking for feedback as it's only a draft PR for now. I haven't formatted and there are some extra prints here and there that I used.

Checklist

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

partial work for issue jaegertracing#4949
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.

approach looks good

Comment on lines +166 to +172
if aH.handleError(w, err, http.StatusBadRequest) {
return
}

if aH.handleError(w, err, http.StatusInternalServerError) {
return
}
Copy link
Member

Choose a reason for hiding this comment

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

only one of these is needed

Comment on lines +55 to +56
fmt.Println(err)
return nil, fmt.Errorf("cannot marshal OTLP : %w", err)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
fmt.Println(err)
return nil, fmt.Errorf("cannot marshal OTLP : %w", err)
return nil, fmt.Errorf("cannot unmarshal OTLP: %w", err)

fmt.Println(otlp_traces.ResourceSpans())
if err != nil {
fmt.Println(err)
return nil, fmt.Errorf("cannot marshal OTLP : %w", err)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
return nil, fmt.Errorf("cannot marshal OTLP : %w", err)
return nil, fmt.Errorf("cannot transform OTLP to Jaeger: %w", err)

@@ -597,6 +597,22 @@ func (*Batch) ProtoMessage() {}
func (*Batch) Descriptor() ([]byte, []int) {
return fileDescriptor_4c16552f9fdb66d8, []int{6}
}

func (m *Batch)ConvertToTraces()(*Trace){
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 auto-generated file, cannot change it manually. Also, I am not sure why you need it - batch is just a collection of spans with a shared Process, and the HTTP handler already has code to translate a list of spans into UI-compatible output.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the fromDomain function that is used by convertModelToUI tries to deref Span.Process (I get a nil pointer derefencing error) :

processID := json.ProcessID(processes.getKey(span.Process))
and doesn't consider a shared common Process, would it be better to modify the fromDomain function instead?

Copy link
Member

Choose a reason for hiding this comment

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

you need to prepare data for that function by denormalizing Batch.Process -> Span.Process

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is what im trying to do in this function by copying m.Process into v.process

Copy link
Member

Choose a reason for hiding this comment

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

I know, but (a) you cannot do this in this file, and (b) that's not all you're doing, L607-610 don't make sense, it's already being done on the GET /v1/trace code path.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ahhh, that makes sense, thanks! I'll revert back with the changes :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yikes! I forgot models.pb is a file generated by protoc from grpc definitions...

@@ -158,6 +161,39 @@ func (aH *APIHandler) formatRoute(route string, args ...interface{}) string {
return fmt.Sprintf("/%s"+route, args...)
}

func (aH *APIHandler) transformOTLP(w http.ResponseWriter, r *http.Request) {
Copy link
Member

Choose a reason for hiding this comment

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

please move the function to a place in the file so that it reflects its relative position in RegisterRoutes

@@ -121,6 +123,7 @@ func NewAPIHandler(queryService *querysvc.QueryService, tm *tenancy.Manager, opt
// RegisterRoutes registers routes for this handler on the given router
func (aH *APIHandler) RegisterRoutes(router *mux.Router) {
aH.handleFunc(router, aH.getTrace, "/traces/{%s}", traceIDParam).Methods(http.MethodGet)
aH.handleFunc(router, aH.transformOTLP, "/transform").Methods(http.MethodPost)
Copy link
Member

@yurishkuro yurishkuro Jan 23, 2024

Choose a reason for hiding this comment

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

Please move down, after aH.getOperationsLegacy

Comment on lines +52 to +53
ptrace_unmarshaler := ptrace.JSONUnmarshaler{}
otlp_traces, err := ptrace_unmarshaler.UnmarshalTraces(OTLPSpans)

Choose a reason for hiding this comment

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

Suggested change
ptrace_unmarshaler := ptrace.JSONUnmarshaler{}
otlp_traces, err := ptrace_unmarshaler.UnmarshalTraces(OTLPSpans)
ptraceUnmarshaler := ptrace.JSONUnmarshaler{}
otlpTraces, err := ptrace_unmarshaler.UnmarshalTraces(OTLPSpans)

same applies to jaeger_traces and jaeger_traces

context: https://go.dev/doc/effective_go#mixed-caps

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.

Ability to upload OTLP data into UI
3 participants