Pass startTime & endTime in Trace Get request

[rim99]

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]

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]

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]

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]

I have a PR #6233

[rim99]

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.

Just remembered this discussion, it looks like Jaeger has adopted nano level of timestamp in HTTP requests already. My memory was incorrect before.

Considering to update the time field as type of fixed64 start_time_unix_nano?