GRPC trace API #199

esnible · 2021-12-23T20:27:18Z

Signed-off-by: Ed Snible [email protected]

This PR creates an Open Telemetry gRPC endpoint for the method /opentelemetry.proto.collector.trace.v1.TraceService/Export. This proof-of-concept does not (yet) contain RBAC security or tenancy. This version uses https://github.com/mwitkow/grpc-proxy to proxy the gRPC without deserializing it.

Previously I created pull 196 which exposed the OTLP Trace HTTP endpoint. That was straightforward using the Chi-router.

The final implementation needs RBAC and tenancy. I haven't yet found anything that is the equivalent to the chi.Router for gRPC. Maybe https://github.com/grpc-ecosystem/go-grpc-middleware ?

It is too early to review this code, beyond the architectural ideas and the choice of libraries, parameter names, etc.

go.mod

matej-g · 2022-01-12T14:08:56Z

Thanks for this @esnible 👍 I've pinged people to take a look at this, I'll be back with my review as well.

esnible · 2022-01-24T14:40:07Z

@matej-g Any feedback?

matej-g · 2022-01-25T13:27:03Z

Hey @esnible,
Although there seem to be still a lot of comments and to-do's, I believe this is in essence how we can proceed, i.e. I imagine we want to have a proxy to forward requests and before that we'll need interceptors for every authn / authz (OIDC, OPA etc.). I'd personally say that we can develop this PR further and continue iterate / give feedback? I'm happy to also chat about this on the tomorrow's community call.

I believe @squat could also give us qualified feedback here, as he was initially having thoughts about how to implement the gRPC interceptors (#158).

bwplotka · 2022-01-26T15:08:54Z

Thanks for this and make sure to ping us on the #observatorium CNCF channel or through private means next - that got blocked too long 🙈

I would love if we can add gRPC API if Otel gRPC is more efficient and popular. That should fit perfectly in our space.

In terms of https://github.com/mwitkow/grpc-proxy feel free to use it. I have somehow written permissions and looks like many wants to jump in and help: mwitkow/grpc-proxy#52. Definitely something running in production in many places. (:

In terms of interceptors please use v2 branch and if anything is missing, feel free to put issue on https://github.com/grpc-ecosystem/go-grpc-middleware/

For authorization - this interceptor might be a good start: https://github.com/grpc-ecosystem/go-grpc-middleware/blob/v2/interceptors/auth/auth.go We might want to convert some middleware to interceptors: https://github.com/observatorium/api/blob/main/main.go#L552

Overall all decisions here makes sense to me! 💪🏽

esnible · 2022-02-04T17:52:30Z

Note: This code requires the 2018 version of mwitkow/grpc-proxy. The repo was untouched; I thought it was orphaned, but just a week ago it got an update that breaks this. I have opened an issue mwitkow/grpc-proxy#54

matej-g · 2022-02-07T08:53:36Z

Note: This code requires the 2018 version of mwitkow/grpc-proxy. The repo was untouched; I thought it was orphaned, but just a week ago it got an update that breaks this. I have opened an issue mwitkow/grpc-proxy#54

What seems to be the issue exactly with using latest version? On a quick look, the changes seem to be more on the surface to modernize the codebase, rather than changing how the library works.

My thinking is, if we're already introducing this dependency and it has been just updated, I would not make these changes to require to use the old version.

matej-g

Thanks for this @esnible! I think the direction is looking good.

It would also be great to have E2E tests for tracing API, but I'm happy to have them after the fact (as long as we don't forget about it 🙂).

api/traces/v1/api.go

matej-g · 2022-02-07T10:07:26Z

api/traces/v1/api.go

+		if fullMethodName == traceRoute {
+			var err error
+
+			if conn == nil {


Do we know what happens if DialContext below errors out? Is conn guaranteed to be nil in such case? I'm thinking about a case when we dial returns error, but because conn is not nil, we would not try to connect again on the second method call.

Also, is there any downside to dialing and obtaining conn outside of the director method? I would find that to be a bit simpler and if there's problem with dialing to collector, we can return error before we even enter director logic.

@matej-g I am now explicitly setting conn to nil on failure.

I haven't tested this code yet for performance or reliability. It passes my simple test: Observatorium-api forwards a trace. Even if the OTel collector is restarted, and the Observatorium-api forwards the next trace. More testing is needed for correct keep-alives and for the OTel collector restarting while Observatorium-api is streaming traces.

matej-g · 2022-02-07T10:14:00Z

main.go

+
+//nolint:lll
+func gRPCServer(cfg *config, tenantHeader string, tenantIDs map[string]string, pm *authentication.ProviderManager, authorizers map[string]rbac.Authorizer, logger log.Logger) (*grpc.Server, error) {
+	director := tracesv1.NewHandler(cfg.traces.writeEndpoint,


I'm wondering whether in our case a simple NewProxy would not suffice instead of introducing director. The way I see it, we don't need to necessarily intercept the method, as this is something that collector would refuse if e.g. a user tries to use unimplemented method.

This PR only handles incoming traces. However, Jaeger supports both HTTP and gRPC query APIs. There will be different RBAC security for forwarding traces and queries.

main.go

authentication/oidc.go

matej-g · 2022-02-07T13:08:06Z

authentication/oidc.go

+			token = authorization[1]
+		}
+
+		idToken, err := a.verifier.Verify(oidc.ClientContext(ctx, a.client), token)


It looks like from this line below, the code is very similar with how HTTP middleware handles this. Do you think we could extract those parts and reuse a single method for both HTTP and gRPC middleware?

authentication/mtls.go

main.go

esnible · 2022-02-09T01:11:25Z

@matej-g I have spent two days trying to get the latest version to proxy OpenTelemetry protobufs through gRPC. Sending data either with grpcurl or with the Open Telemetry gRPC trace exporter gives similar problems. I discuss the problem at mwitkow/grpc-proxy#55

I am extremely concerned about requiring a back-level mwitkow/grpc-proxy. I am looking for a work-around.

What seems to be the issue exactly with using latest version? On a quick look, the changes seem to be more on the surface to modernize the codebase, rather than changing how the library works.

My thinking is, if we're already introducing this dependency and it has been just updated, I would not make these changes to require to use the old version.

matej-g · 2022-02-09T10:47:15Z

@matej-g I have spent two days trying to get the latest version to proxy OpenTelemetry protobufs through gRPC. Sending data either with grpcurl or with the Open Telemetry gRPC trace exporter gives similar problems. I discuss the problem at mwitkow/grpc-proxy#55

Understood, thanks for putting in the effort @esnible 👍, then let's see if get an answer from upstream and in the meantime we can perhaps settle the remaining questions.

frzifus · 2022-02-09T16:25:41Z

api/traces/v1/api.go

+					// because the codec we need to register is also deprecated.  A better fix, if Google removes
+					// the deprecated type, is https://github.com/mwitkow/grpc-proxy/pull/48
+					grpc.WithCodec(grpcproxy.Codec()), // nolint: staticcheck
+					grpc.WithInsecure(),               // nolint: staticcheck


Suggested change

grpc.WithInsecure(), // nolint: staticcheck

grpc.WithTransportCredentials(insecure.NewCredentials())

secondary, but so the linter would give us a green light.

esnible · 2022-02-09T21:24:04Z

@matej-g I would also like to have tests. I see tests in the test/e2e directory. I did not understand them. Can you point me to some docs on test writing?

I have some ideas on how to test the GRPC piece end-to-end with just 2 Open Telemetry agents -- no actual Jaeger or span storage. Would this be desirable? Or should the E2E tests install the full Jaeger, possibly including ElasticSearch, and be as close as possible to a valid deployment?

It would also be great to have E2E tests for tracing API, but I'm happy to have them after the fact (as long as we don't forget about it 🙂).

esnible · 2022-02-17T21:36:44Z

@matej-g I discovered a bug where the Observatorium-api gets an interrupt and restarts every few minutes. I am looking into it; please review but don't merge yet. I also notice that it is checking the tenancy header before checking the authorization header, letting unauthenticated users guess tenant names.

matej-g · 2022-02-28T11:01:08Z

@matej-g I would also like to have tests. I see tests in the test/e2e directory. I did not understand them. Can you point me to some docs on test writing?

We are using the https://github.com/efficientgo/e2e framework, you'll find everything documented there.

bwplotka

Thanks

Overall, functionally makes sense. Let us know if you need help in setting e2e tests. Code looks fine too, just make sure to not handle error twice. If you see something like

foo, err := bar()
if err != nil {
  // produce error/warning/debug log line
  return err
}

That is generally quite bad. The reason is that the passed error is most likely handled in some way - this some way is usually to log it on server side, put metric, return error to user. If every function would add log line when returning error we have lots of wasted information and effort.

HTTP middleware are tricky as usually indeed the returned error is handled by returning HTTP/gRPC response to client/user. But in order to log this I would rather implement logging middleware/interceptor than spending 100 code lines to add log line to every error handling. WDYT?

api/traces/v1/api.go

bwplotka · 2022-02-28T11:38:13Z

api/traces/v1/api.go

+	}
+}
+
+// NewHandler creates the new traces v1 handler.


Suggested change

// NewHandler creates the new traces v1 handler.

// NewOTelConnection creates new GRPC connection to Otel handler.

bwplotka · 2022-02-28T11:40:04Z

api/traces/v1/api.go

+}
+
+// HandlerOption modifies the handler's configuration.
+type HandlerOption func(h *handlerConfiguration)


Is it really handler? This looks like NewOtelConnection looks like client rather?

bwplotka · 2022-02-28T11:40:20Z

api/traces/v1/api.go

+
+const TraceRoute = "/opentelemetry.proto.collector.trace.v1.TraceService/Export"
+
+type handlerConfiguration struct {


Suggested change

type handlerConfiguration struct {

type connOptions struct {

bwplotka · 2022-02-28T11:40:57Z

api/traces/v1/api.go

+		o(c)
+	}
+
+	level.Info(c.logger).Log("msg", "gRPC dialing OTel collector", "endpoint", write)


Is this collector client? I would not presume what's the end target here. End target for this connection might be anything that supports Otel gRPC, no?

bwplotka · 2022-02-28T12:00:53Z

main.go

+	connOtel, err := tracesv1.NewOTelConnection(cfg.traces.writeEndpoint,
+		tracesv1.WithLogger(logger),
+	)


Suggested change

connOtel, err := tracesv1.NewOTelConnection(cfg.traces.writeEndpoint,

tracesv1.WithLogger(logger),

)

connOtel, err := tracesv1.NewOTelConnection(

cfg.traces.writeEndpoint,

tracesv1.WithLogger(logger),

)

bwplotka · 2022-02-28T12:01:15Z

main.go

+}
+
+//nolint:lll
+func gRPCServer(cfg *config, tenantHeader string, tenantIDs map[string]string, pmis authentication.GRPCMiddlewareFunc, authorizers map[string]rbac.Authorizer, logger log.Logger) (*grpc.Server, error) {


Suggested change

func gRPCServer(cfg *config, tenantHeader string, tenantIDs map[string]string, pmis authentication.GRPCMiddlewareFunc, authorizers map[string]rbac.Authorizer, logger log.Logger) (*grpc.Server, error) {

func newOtelGRPCServer(cfg *config, tenantHeader string, tenantIDs map[string]string, pmis authentication.GRPCMiddlewareFunc, authorizers map[string]rbac.Authorizer, logger log.Logger) (*grpc.Server, error) {

bwplotka · 2022-02-28T12:01:46Z

main.go

+
+	// Currently we only proxy TraceService/Export to OTel collectors.
+	// In the future we will pass queries to Jaeger, and possibly other
+	// gRPC methods for logs and metrics to different connections


Suggested change

// gRPC methods for logs and metrics to different connections

// gRPC methods for logs and metrics to different connections.

bwplotka · 2022-02-28T12:10:34Z

main.go

+		// the deprecated type, is https://github.com/mwitkow/grpc-proxy/pull/48
+		grpc.CustomCodec(grpcproxy.Codec()), // nolint: staticcheck
+		grpc.UnknownServiceHandler(grpcproxy.TransparentHandler(director)),
+		// Authorization (tenant)


I think it's visible what interceptors are, we could remove this comment.

bwplotka · 2022-02-28T12:11:03Z

main.go

+			authentication.WithGRPCTenantInterceptors(logger, pmis),
+			auth.StreamServerInterceptor(
+				authorization.WithGRPCAuthorizers(authorizers, gRPCRBAC, logger)),
+			// Log when a trace cannot be forwarded


Signed-off-by: Ed Snible <[email protected]>

matej-g

Thanks for addressing my comments @esnible, this is looking nicely 👍, I have few more comments but I would not like to stall this PR any further. I'll be happy to approve once you get a chance to respond to the latest review.

matej-g · 2022-03-02T16:20:55Z

api/traces/v1/api.go

+	// service supporting opentelemetry.proto.collector.trace.v1.TraceService
+	level.Info(c.logger).Log("msg", "gRPC dialing OTel", "endpoint", write)
+
+	ctx, cancel := context.WithTimeout(context.Background(), 60*time.Second)


Any reason for the 1 minute time out here? Could we do with shorter time out?

authentication/grpc.go

matej-g · 2022-03-02T16:42:27Z

authentication/oidc.go

+		// it lets the caller see messages such as
+		// "failed to authenticate: oidc: token is expired (Token Expiry: 2022-02-03 14:05:36 -0500 EST)"
+		// which are super-helpful in troubleshooting.
+		return ctx, fmt.Sprintf("%s: %v", "failed to authenticate", err),


I think the HTTP behavior might be by design, see discussion here #235 (comment), so worth considering if the same should be apply here?

main.go

Signed-off-by: Ed Snible <[email protected]>

matej-g

Thanks you @esnible! This is look good to me now!

bwplotka

Thanks! It looks definitely good enough 👍🏽

api/traces/v1/api.go

esnible changed the title ~~proof-of-concept~~ GRPC trace API proof-of-concept Dec 23, 2021

esnible changed the title ~~GRPC trace API proof-of-concept~~ [WORK IN PROGRESS] GRPC trace API proof-of-concept Dec 23, 2021

pavolloffay reviewed Jan 3, 2022

View reviewed changes

go.mod Show resolved Hide resolved

esnible changed the title ~~[WORK IN PROGRESS] GRPC trace API proof-of-concept~~ GRPC trace API proof-of-concept Jan 12, 2022

esnible marked this pull request as draft January 12, 2022 14:07

matej-g requested a review from a team January 12, 2022 14:08

esnible marked this pull request as ready for review February 3, 2022 20:55

esnible changed the title ~~GRPC trace API proof-of-concept~~ GRPC trace API Feb 3, 2022

esnible force-pushed the grpc-traces-api branch 2 times, most recently from 7f9346b to 3d5880f Compare February 4, 2022 20:04

matej-g reviewed Feb 7, 2022

View reviewed changes

frzifus reviewed Feb 9, 2022

View reviewed changes

bwplotka reviewed Feb 28, 2022

View reviewed changes

esnible added 6 commits March 1, 2022 09:45

gRPC trace receive implementation

087d262

Signed-off-by: Ed Snible <[email protected]>

Revert to 2018 version of mwitkow/grpc-proxy

1dfb0cb

Signed-off-by: Ed Snible <[email protected]>

Bypass lint to pass lint

ab19134

Signed-off-by: Ed Snible <[email protected]>

Cleaup and refactor OIDC checker to be transport independent

5fd00f1

Signed-off-by: Ed Snible <[email protected]>

Regenerate README

fcc94c0

Signed-off-by: Ed Snible <[email protected]>

lint

287ddd5

Signed-off-by: Ed Snible <[email protected]>

esnible added 4 commits March 1, 2022 09:47

Refactor director and rbac to be extensible for non-OTel connections

9e331d2

Signed-off-by: Ed Snible <[email protected]>

Remove flag for OTel CA, which is not yet supported

9cf1228

Signed-off-by: Ed Snible <[email protected]>

lint

0fb01b5

Signed-off-by: Ed Snible <[email protected]>

Rebase and address code style comments

7d4e3ab

Signed-off-by: Ed Snible <[email protected]>

esnible force-pushed the grpc-traces-api branch from 861409c to 7d4e3ab Compare March 1, 2022 19:47

matej-g reviewed Mar 2, 2022

View reviewed changes

Address code style comments

99c14bf

Signed-off-by: Ed Snible <[email protected]>

matej-g approved these changes Mar 3, 2022

View reviewed changes

bwplotka approved these changes Mar 3, 2022

View reviewed changes

api/traces/v1/api.go Show resolved Hide resolved

bwplotka merged commit b3a1918 into observatorium:main Mar 3, 2022

This was referenced Mar 10, 2022

Update API service image observatorium/observatorium#459

Merged

Expose traces jsonnet configuration #247

Merged

Add tracing jsonnet observatorium/observatorium#461

Merged

pavolloffay mentioned this pull request Mar 18, 2022

Support compression for the public grpc API #255

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GRPC trace API #199

GRPC trace API #199

esnible commented Dec 23, 2021 •

edited

Loading

matej-g commented Jan 12, 2022

esnible commented Jan 24, 2022

matej-g commented Jan 25, 2022

bwplotka commented Jan 26, 2022

esnible commented Feb 4, 2022 •

edited

Loading

matej-g commented Feb 7, 2022 •

edited

Loading

matej-g left a comment

matej-g Feb 7, 2022

esnible Feb 9, 2022

matej-g Feb 7, 2022

esnible Feb 8, 2022

matej-g Feb 7, 2022

esnible commented Feb 9, 2022

matej-g commented Feb 9, 2022

frzifus Feb 9, 2022

esnible commented Feb 9, 2022

esnible commented Feb 17, 2022 •

edited

Loading

matej-g commented Feb 28, 2022

bwplotka left a comment

bwplotka Feb 28, 2022

bwplotka Feb 28, 2022

bwplotka Feb 28, 2022

bwplotka Feb 28, 2022

bwplotka Feb 28, 2022

bwplotka Feb 28, 2022

bwplotka Feb 28, 2022

bwplotka Feb 28, 2022

bwplotka Feb 28, 2022

matej-g left a comment

matej-g Mar 2, 2022

matej-g Mar 2, 2022

matej-g left a comment

bwplotka left a comment

	grpc.WithInsecure(), // nolint: staticcheck
	grpc.WithTransportCredentials(insecure.NewCredentials())

	// NewHandler creates the new traces v1 handler.
	// NewOTelConnection creates new GRPC connection to Otel handler.


		const TraceRoute = "/opentelemetry.proto.collector.trace.v1.TraceService/Export"

		type handlerConfiguration struct {

	func gRPCServer(cfg config, tenantHeader string, tenantIDs map[string]string, pmis authentication.GRPCMiddlewareFunc, authorizers map[string]rbac.Authorizer, logger log.Logger) (grpc.Server, error) {
	func newOtelGRPCServer(cfg config, tenantHeader string, tenantIDs map[string]string, pmis authentication.GRPCMiddlewareFunc, authorizers map[string]rbac.Authorizer, logger log.Logger) (grpc.Server, error) {

	// gRPC methods for logs and metrics to different connections
	// gRPC methods for logs and metrics to different connections.

GRPC trace API #199

GRPC trace API #199

Conversation

esnible commented Dec 23, 2021 • edited Loading

matej-g commented Jan 12, 2022

esnible commented Jan 24, 2022

matej-g commented Jan 25, 2022

bwplotka commented Jan 26, 2022

esnible commented Feb 4, 2022 • edited Loading

matej-g commented Feb 7, 2022 • edited Loading

matej-g left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

esnible commented Feb 9, 2022

matej-g commented Feb 9, 2022

Choose a reason for hiding this comment

esnible commented Feb 9, 2022

esnible commented Feb 17, 2022 • edited Loading

matej-g commented Feb 28, 2022

bwplotka left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matej-g left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matej-g left a comment

Choose a reason for hiding this comment

bwplotka left a comment

Choose a reason for hiding this comment

esnible commented Dec 23, 2021 •

edited

Loading

esnible commented Feb 4, 2022 •

edited

Loading

matej-g commented Feb 7, 2022 •

edited

Loading

esnible commented Feb 17, 2022 •

edited

Loading