Improve cardinality of server spans for better segmentation

[SophieDeBenedetto]

Is your feature request related to a problem? Please describe. GitHub has been evaluating running the ebpf sidecar container provided by opentelemetry-go-instrumentation as a solution for auto-instrumenting our Golang services. After deploying our POC, we ultimately found the server spans provided by the opentelemetry-go-instrumentation probes to be too generic for us to find useful. We would also love to see more probes developed for additional Golang dependencies beyond what's present here. We're hoping to eventually engage GitHub engineers in contributing to this project, so we're starting off with this issue to describe the work we feel would benefit our use-case.

I'll provide a little more detail here on what I mean by "generic" spans.

• DB server spans will all be named "DB", ref:

// pkg/instrumentation/bpf/database/sql/probe.go#L174
return &probe.SpanEvent{
        SpanName:    "DB",
        StartTime:   int64(e.StartTime),
        EndTime:     int64(e.EndTime),
        SpanContext: &sc,
        Attributes: []attribute.KeyValue{
            semconv.DBStatementKey.String(query),
        },
        ParentSpanContext: pscPtr,
    }

We'd benefit from support for templatized parameters here to allow span names to be something like DB <operation, to match the db.operation OTel semconv span name. This allows us to at least segment trace data and RED metrics derived from traces by database operation at a resource level.

Similarly, HTTP server spans are named with the HTTP method only, ref:

// pkg/instrumentation/bpf/net/http/server/probe.go#L204
    return &probe.SpanEvent{
        // Do not include the high-cardinality path here (there is no
        // templatized path manifest to reference).
        SpanName:          method,
        StartTime:         int64(e.StartTime),
        EndTime:           int64(e.EndTime),
        SpanContext:       &sc,
        ParentSpanContext: pscPtr,
        Attributes:        attributes,
    }

While I can see the comment here about the concern around span name cardinality, grouping all traces for a given service by resources segmented on HTTP method only is too blunt for us. We want to see the behavior of specific endpoints in a given service. We'd be interested in expanding this behavior to allow for templatized path parameters to be included in HTTP server span names in a way that is secure/doesn't leak PII.

Describe the solution you'd like There's a few avenues we'd like to explore with maintainers here:

• Understanding what work would be required to improve the cardinality of DB and HTTP server span names for better segmentation of trace data.
• Discussing what it would look like to allow ebpf container users to configure span attributes somehow. For example, if a given service is taking advantage of ebpf auto-instrumentation and uses the net/http package, could users provide some configuration to add additional attributes to spans emitted by that probe?
• Lastly, we'd love to see probes developed for even more dependencies, especially twirp, which GitHub uses extensively. I didn't find any issues referencing twirp specifically, but I'd love to chime in if there is a place you're tracking requests for/work on probes for new dependencies.

Additional context I can't commit that GitHub engineers would definitely engage here at this time, but we're trying to generate some interest internally in contributing to this project so I'm hoping this issue can spark some conversation on the topics I've outlined here/any ideas to address these concerns.

Thank you!

cc @arielvalentin who has been working on this internally at GitHub with me 😄

[RonFed]

Hi @SophieDeBenedetto, thank you for this issue, it contains some great points. I think we can improve on the DB and HTTP span names.

• database/sql: I originally thought db.operation should indicate the SQL operation (INSERT, DELETE, ...) as suggested from the semantic conentions and this information is not easy to grab from eBPF without parsing the query. However, I looked into how its done in otelsql, and they use operations specific to database/sql ("sql.conn.query", "sql.conn.exec"). If this approach is acceptable, we can adopt it here as well, and modify the span names.
• net/http: I agree that having the method as the span name is not ideal, however using the current instrumentation I could not find a way to extract a templatized path. In eBPF we have access to the URL struct. Currently I don't see a way of producing something like /api/customer/{custId}/address/{addrId} instead of /api/customer/23476253654/address/237464. If a user wants to opt-in for including the potentially high cardinality span name, we can add an envariable variable for that (similar for the one to collect db queries).
• twirp: It looks like twirp is using net/http so I would expect we should have spans correlating to the underlying http with our current probes. Adding new instrumentation can vary in the time it takes to implement, mainly because it depends on the library api, and how accessible the relevant attributes data. Instrumenting the library itself requires looking into its code and finding the right functions we can probe.

[SophieDeBenedetto]

Hi @RonFed and thanks for your response here! I'll share some thoughts on your points one by one:

• database/sql -> what's you're suggesting to get parity with otelsql and set span name to the database/sql operation sounds like a great improvement to us. Wherever we can get parity with existing opentelemetry instrumentation behaviour, we'd love to see that.
• net/http -> yes indeed this is a hard problem to solve. I don't think we'd be opposed to opting into a feature flag like the one your describing but I agree it doesn't really solve the problem. The OTel spec here describes HTTP span names like this:

  HTTP server span names SHOULD be {method} {http.route} if there is a (low-cardinality) http.route available (see below for the exact definition of the {method} placeholder).

But the challenge of how to detect a low-cardinality http.route remains. One thing I'm thinking of is the span name formatters that [opentelemetry-go-contrib](https://github.com/open-telemetry/opentelemetry-go-contrib/blob/main/instrumentation/net/http/otelhttp/config.go#L44) makes available for users of that package. This allows users to provide their own custom span name formatter to implement logic that could scrub PII data from routes for HTTP span names, and/or recognize high-cardinality vs. low-cardinality routes for span names. Has any thought been given to exposing some similar level that users can pull with using ebpf auto-instrumentation? I confess I don't know what that would take to make a reality.

• twirp instrumentation -> Totally understood that this is something that would take investigation to understand what is needed and time to implement. This is an area where GitHub engineers may be interested in plugging in in the future. I'm going to push this idea on my end but I don't think it would be any time in the immediate future.

Thanks again for engaging with us here and looking forward to hearing any additional thoughts you may want to share!