jaegertracing / jaeger

CNCF Jaeger, a Distributed Tracing Platform
https://www.jaegertracing.io/
Apache License 2.0
20.54k stars 2.44k forks source link

Pass startTime & endTime in Trace Get request #6228

Closed rim99 closed 1 day ago

rim99 commented 2 days ago

Test would fail on proto marshal

Running tool: /usr/bin/go test -timeout 30s -run ^TestGetTraceTraceIDError$ github.com/jaegertracing/jaeger/cmd/query/app/apiv3

--- FAIL: TestGetTraceTraceIDError (0.00s)
    /home/xxx/projects/go/jaeger/cmd/query/app/apiv3/grpc_handler_test.go:132: 
            Error Trace:    /home/xxx/projects/go/jaeger/cmd/query/app/apiv3/grpc_handler_test.go:132
            Error:          Received unexpected error:
                            rpc error: code = Internal desc = grpc: error while marshaling: proto: time.Time does not implement Marshal
            Test:           TestGetTraceTraceIDError
FAIL
FAIL    github.com/jaegertracing/jaeger/cmd/query/app/apiv3 0.007s
FAIL
yurishkuro commented 2 days ago

Thanks, I see why this test is failing and know how to fix it. However, it raises an interesting question about which data type we should use for the timestamps in the API. In api_v2 we used google.protobuf.Timestamp, so api_v3 was trying to replicate that. However, in api_v2 the Span object also uses google.protobuf.Timestamp, whereas in v3 we are using OpenTelemetry Span type which represents timestamps as fixed int64

  fixed64 start_time_unix_nano = 7;

So while our api_v2 was consistent between data model and the service interface, api_v3 is not consistent.

rim99 commented 2 days ago

I see, but unix nano level of timestamp seems to be too granular for API queries.

Current HTTP API uses micro seconds level of timestamp for "loopback". I think we'd better have a very good reason to break it

yurishkuro commented 2 days ago

I agree, I don't really want to change api_v3 as it has been declared stable a long time ago. So we will continue using google.protobuf.Timestamp, we just need to fix some annotations in the IDL and make a change in the Go repository. Specifically, the IDL file was missing these annotations:

// Enable gogoprotobuf extensions (https://github.com/gogo/protobuf/blob/master/extensions.md).
// Enable custom Marshal method.
option (gogoproto.marshaler_all) = true;
// Enable custom Unmarshal method.
option (gogoproto.unmarshaler_all) = true;
// Enable custom Size method (Required by Marshal and Unmarshal).
option (gogoproto.sizer_all) = true;

And then in the Go code I had to add a MarshalToSizedBuffer function to the Traces struct.

Do you want to try to make these changes?

yurishkuro commented 1 day ago

I have a PR https://github.com/jaegertracing/jaeger/pull/6233