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.

Sign up for GitHub

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

Jump to bottom

Define internal version of OTLP proto types using Gogo optimizations #5052

Closed
2 of 5 tasks
Tracked by #4843
yurishkuro opened this issue Dec 27, 2023 · 0 comments · Fixed by #5067
Closed
2 of 5 tasks
Tracked by #4843

Define internal version of OTLP proto types using Gogo optimizations #5052

yurishkuro opened this issue Dec 27, 2023 · 0 comments · Fixed by #5067
Assignees
@yurishkuro

Comments

@yurishkuro
Copy link
Member

yurishkuro commented Dec 27, 2023 •
edited
Loading

Per Jaeger v2 design doc, we need to have our internal copy of code-generated Proto types for OTLP data model (because OTel Collector keeps it private). My current experiments in https://github.com/yurishkuro/jaeger/tree/otel-proto-gogo ran into an issue with grpc-gateway that already uses internally built OTLP protos, but without Gogo optimizations.

Plan of action

  • Upgrade grpc-gateway to v2
  • Try to use v2 gateway with gogoproto marshaling
    • if it does not work, consider reimplementing the gateway manually, it's only 4 functions in the API. It will also be more efficient since we won't need to do a roundtrip to the gRPC service.
@yurishkuro yurishkuro self-assigned this Dec 27, 2023
This was referenced Dec 27, 2023
Merged
Merged
Open
yurishkuro added a commit that referenced this issue Dec 28, 2023
@yurishkuro
Add grpc-gateway tests for all APIv3 methods (#5051)
bdd43f7 
## Which problem is this PR solving?
- Part of #5052
- Continuation of #5046

## Description of the changes
- Add tests for all HTTP APIs, not just GetTrace
- Use snapshots to make validation of HTTP/JSON response from the server
easier
- Replace grpc-gateway/runtime JSONPb marshaler (in tests) with
gogo/jsonpb marshaler

## Gaps
- The error conditions are not being tested currently, such as not
specifying the timestamps in the query

## How was this change tested?
- Unit tests

---------

Signed-off-by: Yuri Shkuro <[email protected]>
This was referenced Dec 28, 2023
Merged
Merged
yurishkuro added a commit that referenced this issue Dec 30, 2023
@yurishkuro
Add manual implementation of APIv3 HTTP endpoints (#5054)
718f666 
## Which problem is this PR solving?
- Part of #5052

## Description of the changes
- Pull in IDL change
jaegertracing/jaeger-idl#102
- Re-implement APIv3 HTTP endpoints without the use of grpc-gateway
- Share tests from grpc-gateway for manual implementation
  - Refactor tests to avoid duplication of snapshots
- Fix inconsistency between http and grpc tenancy interceptors where
HTTP was returning Unauthenticated in certain cases, but GRPC was always
returning Forbidden. Make them consistent: missing tenant header results
in Unauthenticated.

## Follow-ups
- [x] http implementation needs more unit tests (mostly error handling
and parameter variations)
- [ ] the new implementation is not hooked up into production code yet,
I first want to confirm it works with model-v2, and just in general
minimize the scope of a single PR

## How was this change tested?
- Using unit tests added in #5051, with additional enhancements

---------

Signed-off-by: Yuri Shkuro <[email protected]>
yurishkuro added a commit that referenced this issue Dec 30, 2023
@yurishkuro
Remove grpc-gateway dependency (#5060)
003111b 
## Which problem is this PR solving?
- Part of #5052
- Continues #5054
- Closes #4911

## Description of the changes
- Replace grpc-gateway based implementation with manual HTTP
implementation from #5054
- Clean up spurious grpc-gateway usage (e.g. all-in-one test that did
not need it)
- Delete grpc-gateway step from `make proto` and remove the
corresponding generated file
- `go mod tidy` removes grpc-gateway and github.com/golang/protobuf

## How was this change tested?
- CI

---------

Signed-off-by: Yuri Shkuro <[email protected]>
@yurishkuro yurishkuro mentioned this issue Dec 31, 2023
Merged
yurishkuro added a commit that referenced this issue Jan 4, 2024
@yurishkuro
Add gogo/protobuf to OpenTelemetry OTLP data model (#5067)
a7c5ef0 
## Which problem is this PR solving?
- Resolves #5052

## Description of the changes
- Apply gogo annotations to OTLP proto for generating more efficient
data model

## How was this change tested?
- Unit tests

---------

Signed-off-by: Yuri Shkuro <[email protected]>
@yurishkuro yurishkuro moved this to Done in Jaeger V2 Feb 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@yurishkuro yurishkuro

Labels
None yet
Projects
Jaeger V2
Archived in project
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Add gogo/protobuf to OpenTelemetry OTLP data model yurishkuro/jaeger
1 participant
@yurishkuro