Financial-Times / dotcom-reliability-kit

🪨 A well tested suite of tools designed to help FT.com applications be more reliable and measurable
MIT License
7 stars 1 forks source link

Adding additional metadata for fetch requests #901

Open GlynnPhillips opened 7 months ago

GlynnPhillips commented 7 months ago

The ability to add additional metadata to fetch requests

What problem does this feature solve?

A couple of scenarios I think this would help for

  1. When using GraphQL endpoints it would be useful to add a datapoint that would help to identify what query is being made as currently the tracing doesn't include the body (query)
  2. Being able to identify requests by users who have a flag/AB test enabled

Ideal solution

I haven't given it lots of thought but the first idea that came to mind for me was having a reserved header prefix so any fetch requests can add a header and that is added to the tracing logs. Something like

fetch(url, {
    headers: {
        'x-opentelemetry-keyname': 'value'
    }
});

Alternatives

🤷‍♂️ I am not familiar enough with this software to know if my suggestion would even work so alternatives are a stretch 😄

rowanmanning commented 7 months ago

Thanks @GlynnPhillips. I think we're definitely gonna do this but need to investigate what options we have 👍 I'm moving it into the "planned" column but for anyone picking this up we probably need to discuss as a group before implementing anything.

rowanmanning commented 7 months ago

Ooh this might be easier than we thought code-wise 🙂 the HTTP instrumentation has an option named headersToSpanAttributes (types). This option can be set like this:

getNodeAutoInstrumentations({
    // ...
    '@opentelemetry/instrumentation-http': {
        // ...
        headersToSpanAttributes: {
            client: {
                requestHeaders: [
                    'example-header'
                ],
                responseHeaders: [
                    'example-header'
                ]
            },
            server: {
                requestHeaders: [
                    'example-header'
                ],
                responseHeaders: [
                    'example-header'
                ]
            }
        }
    },
    // ...
})

Maybe we should do some experimenting locally to see if this gives us what we want. Then we could work together to come up with a list of headers that we think would be useful and decide if we want to introduce some custom headers for metadata.

GlynnPhillips commented 7 months ago

Ohh that looks good. I am happy to help pair on this if you want/need at somepoint

rowanmanning commented 7 months ago

I've had a little go at this to see what the data looks like. The data comes through as a span attribute but not an annotation which is what we'd need to be able to query by these values. Also headers are always sent as an array which might complicate things a little, e.g. the trace span metadata looks like this:

{
    // ...
    "http.response.header.example_server_response": [
        "header-value"
    ],
    // ...
}

The above is the result of setting headersToSpanAttributes.server.responseHeaders to include "example-server-response". You can see an example trace here~context~(xrayGroup~'))).

If we want to add actual annotations then I think we might need to either:

rowanmanning commented 2 months ago

@GlynnPhillips is it possible this could be solved better with custom metrics now? E.g have a counter for each type of query along with timings and failure states.