Response/Request Size GRPC Metrics Instrumentation

[asweet-confluent]

Adds rpc.request.body.size and rpc.response.body.size attributes. This is just an updated version of #11833.

[asweet-confluent]

Does anyone know why the server spans are using request size for rpc.response.body.size and response size for rpc.request.body.size? This is how the original PR was written.

CC @crossoverJie

[crossoverJie]

Sorry, I'm not really sure about the specific reasons, you can check the commit history of this repo.

However, the previous PR was blocked by this PR, maybe it can be re-pushed.

[jaydeluca]

out of curiosity, are the two of you working together on this? I just want to understand why we have two PRs for the same thing and make sure we're all on the same page. I think the reason the other PR stalled is because there were some open questions about semantic conventions, and as @crossoverJie said we might need to revisit and push on that before we can move forward with this.

[asweet-confluent]

are the two of you working together on this?

No, the other PR is just very out of date I and I wanted to get this done so I rebased it myself. I assumed it was abandoned :)

I think the reason the other PR stalled is because there were some open questions about semantic conventions, and as @crossoverJie said we might need to revisit and push on that before we can move forward with this.

AFAIK these are already in the semantic conventions:

• server
• client

[laurit]

AFAIK these are already in the semantic conventions

What you have in the semantic conventions is rpc.server.request.size, rpc.server.response.size, rpc.client.request.size and rpc.client.response.size metrics. Semantic conventions don't seem to define the span attributes rpc.request.body.size and rpc.response.body.size. Either these attributes would need to be added to semantic conventions or the api that is used to produce the metrics would need to be changed so that it could include values that are not span attributes. Our convention is not to emit telemetry that is not in the spec in default configuration.
To me

                                    equalTo(RPC_RESPONSE_BODY_SIZE, requestSerializedSize),
                                    equalTo(RPC_REQUEST_BODY_SIZE, requestSerializedSize)

looks like a bug since both the request and response size are the same.

[asweet-confluent]

Got it - how about I rip out the code for trace attributes and just leave the metrics?

[asweet-confluent]

@jaydeluca looks like the failing tests are dead links unrelated to these changes. I'm still unsure of this though.

[otelbot-java-instrumentation]

🔧 The result from spotlessApply was committed to the PR branch.

[otelbot-java-instrumentation]

🔧 The result from spotlessApply was committed to the PR branch.

[asweet-confluent]

@laurit @jaydeluca I removed the payload size attributes from the traces so we're just left with the metrics. The original implementation worked by adding the attributes to the traces, then using those attributes to record the metrics. It looks like there's no straightforward way for a OperationMetrics to access the requests directly, so I ended up using a context customizer to store the payload size and access it in the Rpc*Metrics classes. Let me know if there's a better approach to this; I had a hard time understanding all the different abstractions at play.

[jaydeluca]

Unless there is a reason not to (for example, many unrelated attributes we don't necessarily care about), we typically use the more stricter assertion

Suggested change

	.hasAttributesSatisfying(
	.hasAttributesSatisfyingExactly(

[laurit]

@trask an alternative would be to introduce an OperationAttributeExtractor we could call into it in

opentelemetry-java-instrumentation/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/Instrumenter.java

Lines 250 to 256 in 14a5360

	UnsafeAttributes attributes = new UnsafeAttributes();
	for (AttributesExtractor<? super REQUEST, ? super RESPONSE> extractor : attributesExtractors) {
	extractor.onEnd(attributes, context, request, response, error);
	}
	span.setAllAttributes(attributes);
	OperationListener[] operationListeners = context.get(START_OPERATION_LISTENERS);

after the span attributes. We could merge the attributes from onEnd with the ones from OperationAttributeExtractor and pass them to the OperationListener. WDYT?

[trask]

@laurit @jaydeluca I removed the payload size attributes from the traces so we're just left with the metrics. The original implementation worked by adding the attributes to the traces, then using those attributes to record the metrics. It looks like there's no straightforward way for a OperationMetrics to access the requests directly, so I ended up using a context customizer to store the payload size and access it in the Rpc*Metrics classes. Let me know if there's a better approach to this; I had a hard time understanding all the different abstractions at play.

given the complexity this introduces, what about making both the body size span attributes and body size metrics opt-in?

the RPC semconv stability SIG just kicked off yesterday. I suspect we'll get the body size span attribute into RPC semconv, but I also suspect that both it and the body size metrics will remain experimental when RPC semconv initially goes stable (similar to the state of body size span attributes and metrics in HTTP). So both of them will probably need to be opt-in anyways at some point in the future when this repo adopts the first stable RPC semconv release.

[asweet-confluent]

given the complexity this introduces, what about making both the body size span attributes and body size metrics opt-in?

If you're suggesting going back to the original implementation:

adding the attributes to the traces, then using those attributes to record the metrics

Doesn't that mean that enabling the metrics will require enabling the span attributes? Or we'd have to add the span attributes, conditionally generate the metrics from the span attributes, then conditionally remove the span attributes after.

[laurit]

@asweet-confluent I update this PR to use an experimental API that allows adding attributes that are used just for operation listeners.