Normalize HTTP instrumenters options

[albertored]

At the moment we have 3 HTTP server instumneters (cowboy, phoenix and elli) and 4 HTTP client ones (finch, httpoison, req and tesla).
They expose some common option, some options that somehow overlap but are not equal and some options that are specific to that instrumenter.

Proposed options

The set of common options I would like to have on each HTTP instrumenter is the following:

• {req|resp}_headers_to_span_attributes - this allows to give a list of request/response headers to be used as span attributes as per semantic conventions. A PR for adding this is already open, see Allow to configure which headers should be used as span attrs #184

• span_name - this option should be a string (for fixed span names) or a function that takes as input the request (the representation of the request depends on the library) and return the span name.

  TBD if the string value is needed or if it's ok to allow only the function, it should behave like request_hook below.

  This is already implemented (partially) by some instrumenters:

  • httpoison takes ot_span_name with value the actual span name (a string)
  • req takes span_name with value the actual span name (a string)
  • tesla takes span_name with value the actual span name (a string) or a function taking as argument the %Telsa.Env{} struct

• request_hook - function that takes as input the request and returns an enumerable of span attributes to be set on span creation. This is useful to add attributes that can be used for sampling decisions.

  TBD not fully satisfied with the name, it should somehow explain that is returning span attributes.

  TBD should be possible to give a fixed enumerable of attributes instead of a function? It should behave like span_name above.

  This is already implemented (partially) by some instrumenters:

  • cowboy in PR [cowboy instrumenter] Allow users to set custom attributes on span start #128
  • elli has the server_name option to add a specific http.server_name attribute
  • httpoison has the ot_attributes option but is is a fixed map of attributes and not a function. It also has a convoluted logic that takes into account the ot_resource_route and infer_route options to set the http.route span attribute.

• propagate_ctx - boolean, whether to propagate the trae context in outoging requests (only for HTTP clients).

  TBD if a boolean is sufficient or if taking a propagator as input (as done in tesla, see below) can be useful.

  This is already implemented (partially) by some instrumenters:

  • req with the propagate_trace_ctx option (bool)
  • tesla with the propagator option that can be an otel_propagator:t() or the atom :none

Actual extra options

In addition to these options some library expose its own specific ones:

• tesla has the mark_status_ok options to allow setting the span status to ok to request that returns an HTTP status code that is normally mapped as an error. This is against semantic conventions, we should decide if extending this option to other instrumenters or removing it from tesla.
• elli has the exluded_paths option to allow excluding some paths from automatic span creation, may be useful to add it to other instrumenters?
• req has the no_path_params option, whose use is not so clear to me
• phoenix has some specific options that do not need any change (endpoint_prefix and adapter)

[bryannaegele]

tesla has the mark_status_ok options to allow setting the span status to ok to request that returns an HTTP status code that is normally mapped as an error. This is against semantic conventions, we should decide if extending this option to other instrumenters or removing it from tesla.

This is actually spec compliant and is contained in the API spec. Operators are allowed to mark a span as Ok unless explicitly prevented from doing so by another part of the spec. HTTP does not contain any prevention.

https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/trace/api.md#set-status

[albertored]

The specification is very clear, thanks for sharing. I got confused by the HTTP semantic conventions (https://github.com/open-telemetry/semantic-conventions/blob/main/docs/http/http-spans.md#status) where setting span status to error for codes >= 400 is a MUST

[albertored]

If this is the case (the span can be marked as ok as configured) then we should extend this functionality also to other libs and to 2xx and 3xx status codes, now it is allowed only for 4xx and 5xx ones.

[albertored]

Moving here a comment from @tsloughter

Actually, maybe there should be one function that returns whether to create a span and whether to propagate:

filter(Req) -> {boolean(), boolean()}

I actually don't see filtering mentioned at all in this doc? We def need some function or regex (on path) based filtering to allow users to define if spans should be started or not for a request.

Like https://pkg.go.dev/go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp#Filter

[bryannaegele]

If this is the case (the span can be marked as ok as configured) then we should extend this functionality also to other libs and to 2xx and 3xx status codes, now it is allowed only for 4xx and 5xx ones.

You can only mark an error'd span ok. Those codes aren't errors.

[bryannaegele]

I know it's not fun, but you might want to spend some time reading through the General, API, and SDK sections of the spec to get a good understanding of the foundations.

[albertored]

I read it but my understanding of the total order part was "once set to ok the status can't change" not "to set the status to ok it must pass from an error one", thanks for clarifying that to me.

I also hope my previous messages don't sound arrogant, it was not my intention but not being a native English speaker I imagine they can be misread

[bryannaegele]

No worries on the language bit :)

The intent of Ok is to override any marking of the span as errored and prevent any future changes to the status. There's no other benefit and having an explicit option for those defined would likely just cause confusion.