router icon indicating copy to clipboard operation
router copied to clipboard

response_body is not valid under telemetry spans for custom attribute

Open Samjin opened this issue 11 months ago • 8 comments

https://www.apollographql.com/docs/router/configuration/telemetry/instrumentation/selectors/#supergraph It says supergraph has selector of response_body, but the router config schema does not contain it. Add a custom attribute under supergraph would fail.

Expected behavior Expect response_body selector to be valid under either supergraph or router.

Output

telemetry:
  instrumentation:
    spans:
      supergraph:
        attributes:
┌        "x-custom2":
|           response_body: "arg2"
â””-----> {"response_body":"arg2"} is not valid under any of the schemas listed in the 'anyOf' keyword

Samjin avatar Mar 20 '24 23:03 Samjin

it looks like this is happening because response_body should be a JSON path. The error message is not clear enough, this is something we should fix

Geal avatar Mar 25 '24 10:03 Geal

it looks like this is happening because response_body should be a JSON path. The error message is not clear enough, this is something we should fix

Are you saying arg2 should be a JSON path? I think I tried that and didn't work. The schema config definition(aka (configuration_schema.json) doesn't have response_body as valid property either.

Samjin avatar Mar 25 '24 16:03 Samjin

Yep, adding here that under supergraph attributes in the YAML config, we only allow these response attributes (as seen from the generated configuration_schema.json from Router 1.43.1

  • response_header
  • response_context

Yet our docs indicate that we have support for response_body as well.

If we add support for response_body that would align with the docs and yes it should be a JSON selector but we might want to consider doing what we did for subgraph responses too and split that into

  • supergraph_response_data
  • supergraph_response_errors

smyrick avatar Apr 02 '24 23:04 smyrick

Hmm, yes response_body does seem to be missing.

BrynCooke avatar Apr 03 '24 08:04 BrynCooke

The main issue with implementing this is that the response when we get to the supergraph stage is a stream. To support this we will need to extend the Selector trait to add somethign along the lines of on_response_part. This would allow the selector handling to be injected into the stream of responses.

BrynCooke avatar Apr 03 '24 08:04 BrynCooke

Removing from the docs here to reduce confusion until it is actually implemented: https://github.com/apollographql/router/pull/4905

smyrick avatar Apr 03 '24 16:04 smyrick

@bnjjj Is this fixed by your latest PR?

BrynCooke avatar Apr 29 '24 12:04 BrynCooke

Yes it's part of this PR but it will be response_errors and response_data instead.

bnjjj avatar Apr 29 '24 12:04 bnjjj

@bnjjj Should this have been marked as one of the "Fixes" in #5022? (In other words, can it be closed?)

abernix avatar May 17 '24 08:05 abernix

Yes!

bnjjj avatar May 17 '24 09:05 bnjjj