Add query service with OTLP

[pavolloffay]

Signed-off-by: Pavol Loffay p.loffay@gmail.com

Supersedes: #3051
Depends on: jaegertracing/jaeger-idl#76

This PR adds support for a new query gRPC and REST (via grpc-gateway) API. The API definition is in jaegertracing/jaeger-idl#76 and it mimics the current gRPC query API, however it uses the OTLP model.

IDs (trace, span, parent) are encoded as hex strings in JSON (see spec https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/protocol/otlp.md#otlphttp) and hex IDs are also accepted as query params e.g. get trace by id. IDs are defined as byte arrays in proto and by default proto serializes them into base64. This implementation uses gogo with the custom type feature (that overrides marshalling) to overcome this. I am fine to keep using gogo and migrate to custom marshaller/vtprotobuf once OTEL does.

Example REST requests:

curl -ivX GET -H "Content-Type: application/json" http://localhost:16686/v3/traces/000000000000000061677de41fa1e1e5
curl -ivX GET -H "Content-Type: application/json" http://localhost:16686/v3/traces\?query.serviceName\=frontend

Notable changes

• translator from Jaeger model to OTLP
• new GRPC handler
• gRPC gateway registration
• copy of JSONPb - the description why is in the code

The GraphQL API will not be used because:

• serialization of protobuf is problematic
  • protobuf uses JSONPb and not standard JSON annotations
  • custom marshaller could be used but it breaks selecting object fields - it API would always return full size objects
  • we could define a separate model that would mimic use JSONPb serialization, but this adds unnecessary maintenance overhead and we might run into other serialization incompatibilities as well.
• all GraphQL libs seem to be supported by a single maintainer/company

TODOs

• query base path does not work in the grpc-gateway
• add model translator from jaeger/model to OTLP - https://github.com/open-telemetry/opentelemetry-collector/tree/main/translator/trace/jaeger cannot be used bc the OTLP model in the collector has a different package/is in internal package

[pavolloffay]

@yurishkuro, @joe-elliott could you please have a quick look, especially on the proto definition.

[codecov]

Codecov Report

Merging #3086 (0b215b3) into master (e7d7eb7) will decrease coverage by 0.03%.
The diff coverage is 95.51%.

@@            Coverage Diff             @@
##           master    #3086      +/-   ##
==========================================
- Coverage   95.92%   95.89%   -0.04%     
==========================================
  Files         236      239       +3     
  Lines       10238    10525     +287     
==========================================
+ Hits         9821    10093     +272     
- Misses        348      355       +7     
- Partials       69       77       +8     

Impacted Files	Coverage Δ
cmd/query/app/grpc_handler.go	98.62% <ø> (ø)
cmd/query/app/server.go	94.07% <72.72%> (-1.76%)	⬇️
cmd/query/app/apiv3/grpc_gateway.go	86.66% <86.66%> (ø)
cmd/query/app/apiv3/grpc_handler.go	88.23% <88.23%> (ø)
cmd/query/app/apiv3/otlp_translator.go	100.00% <100.00%> (ø)
cmd/query/app/static_handler.go	96.77% <0.00%> (-1.62%)	⬇️
plugin/storage/badger/spanstore/reader.go	95.37% <0.00%> (-0.72%)	⬇️
... and 1 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e7d7eb7...0b215b3. Read the comment docs.

[jpkrohling]

Why do you need to change the package again? Can't you have the service in one package and the OTLP proto in another?

[pavolloffay]

I have added more comments to the makefile

[jpkrohling]

I think otlp would be more accurate.

[pavolloffay]

I would use otlp for the package name where the otlp is compiled. Not for Jaeger's query service. I will use apiv3.

[pavolloffay]

PR updated. It's ready for another round.

[yurishkuro]

Isn't there a ticket in OTEL to export that package? It do we not want to use it?

[pavolloffay]

I didn't see any open issue. It has been private from the get-go.

[joe-elliott]

We have a similar process in Tempo which is here:
https://github.com/grafana/tempo/blob/main/Makefile#L129

We have also since forked the collector and written a script to just rename internal => external so we can have access to more of the otel guts. We will probably move to using this soon instead of our current setup.
https://github.com/grafana/opentelemetry-collector/tree/0.29-grafana

I didn't see any open issue. It has been private from the get-go.

It used to be exposed b/c Tempo vendored it initially. They moved it internal and we asked about keeping it exposed. They opted to keep it internal to protect themselves from people forming a dependency on it.

[pavolloffay]

I didn't see it being open, but good to know. Perhaps they have reasons to keep it intenal. They even spent a lot of time on the (ugly) pdata package to make that happen.

[pavolloffay]

PR updated and coverage is back on green. Please review again.

[pavolloffay]

I gave it a bit more love and added cancelation for the grpc gateway

[albertteoh]

this function looks like it's going to do the majority of work when fetching traces; would it be worth initializing slice capacities where we know the expected sizes to avoid memory realloc. For example, here we could add:

Spans: make([]*v1.Span, len(spans))

same with rss and rs.InstrumentationLibrarySpans below, as well as with some of the other j...ToOTLP funcs which may have the opportunity for this simple optimization.

[pavolloffay]

make([]*v1.Span, len(spans))

Does not seem correct. The function iterates over jaeger spans and creates resource spans slice and instrumentation slice spans. before hand, the size of these two slices is not known.

[albertteoh]

ah okay, although I thought the sizes for rss and rs.InstrumentationLibrarySpans could be determined up front as len(spansByLibrary) & len(libMap) respectively.

[albertteoh]

I learnt from an earlier review that types should be at the top of file.

[pavolloffay]

I think it mostly applies to the exported types. This is a helper type that is bounded to the not exported function below

[albertteoh]

considering this scenario where span0 has a tag:

key = "a"
val = "bc"

and span1 has a tag:
key = "ab"
val = "c"

could these two result in the same hash? if so, what is the consequence?

similarly, for cases where the value is an empty string:

span0: a="", b="c" -> abc
span1: a="b", c="" -> abc

please ignore if these scenarios won't realistically happen :)

[pavolloffay]

corner case :). Then the uniqueness in the map will depend only on the service name which is fine.

[albertteoh]

why is resource used as the key in spansByLibrary if uniqueness depends only on the service name?

[pavolloffay]

uniqueness does not depend only on the service name but as well on the string tags.

[pavolloffay]

can somebody re-review?

[albertteoh]

although I agree that we should not return an error to the API caller or even return early in this function if there is an error in translating status codes, would it be worth logging (even on debug) the error so the problem can be addressed?

I imagine failure to map a jaeger span status code to OTLP status code should be more likely the exception than the norm, right?

[pavolloffay]

It should be an exception state, this is based on OTELcol translator which does not log the error either. Adding logger is not a problem, but it will a bit pollute translator API.

[albertteoh]

related to #2988 (comment), i think something like otlptrace.ResourceSpans is more readable than v1.ResourceSpans. Perhaps the same for commonv1 and resourcev1.

likewise, I think it would be easier to read jaeger.Span than model.Span.

[pavolloffay]

I have renamed it to v1 to tracev1

[albertteoh]

besides needing to rebase, lgtm