Excessive span.AddEvent for request/response size

[jipperinbham]

We just switched to using the module and have been very pleased with the auto-instrumented requests but have also seen the number of events we send to our provider (honeycomb) increase quite a bit. Because their usage is based on number of events, when calling span.AddEvent to attach the request and response, a single request between two services is now sending 5 events instead of 2.

Is there a specific otel convention for using AddEvent to include the request/response sizes? Could we have an option to include them as attributes on the main request span?

[akshayjshah]

Is there a specific otel convention for using AddEvent to include the request/response sizes? Could we have an option to include them as attributes on the main request span?

Including the request and response sizes is part of the OTel RPC conventions. There's no particular reason we need to send any of that data using AddEvent, though.

We'll take a look at this for unary RPCs, but it'll take us a little while to get to it. We're happy to take a PR if you're on a tighter schedule, though :)

[jipperinbham]

We're happy to take a PR if you're on a tighter schedule, though :)

Nice, I’ll see about getting something together soon.

[mariusvniekerk]

The default implementation only optionally enables these events

https://github.com/open-telemetry/opentelemetry-go-contrib/blob/main/instrumentation/google.golang.org/grpc/otelgrpc/config.go#L162-L171

[DavidS-ovm]

@akshayjshah can you please have a peek at marius' PR (https://github.com/bufbuild/connect-opentelemetry-go/pull/118) it seems to solve the problem nicely.

[joshcarp]

So looking into this a but more it seems like the solution i'm leaning towards is https://github.com/bufbuild/connect-opentelemetry-go/pull/98
Generally speaking we want to adhere to the standard as closely as possible, and provide overwrites when the standard doesn't act the way we want it to. This means that adding the events by default and adding an option to ignore them.

The fact that the gRPC implementation doesn't really mean much apart from the fact that it's not adhering strictly to its own standard:

In the lifetime of an RPC stream, an event for each message sent/received on client and server spans SHOULD be created. In case of unary calls only one sent and one received message will be recorded for both client and server spans.

This notably doesn't mention anything about adding the events as attributes on the main span, and if that is the desired behaviour then it should be raised as an issue on the opentelemetry specification.

With that being said https://github.com/bufbuild/connect-opentelemetry-go/pull/98 is approved