[opentelemetry-tesla] add custom span name override as middleware opt

[nirev]

This PR adds a new middleware option so that it's possible to override the span name.

eg:

def client(url) do
  middleware = [
    {Tesla.Middleware.BaseUrl, url},
    {Tesla.Middleware.OpenTelemetry, span_name: "POST :outgoing-webhooks"}
  ]

  Tesla.client(middleware)
end

Changes:

• removed extra bracket from mix.exs, preventing compilation
• added test cases for the three possible span name rules
• added a @moduledoc
• added capture_log: true in tests

[linux-foundation-easycla[bot]]

The committers listed above are authorized under a signed CLA.

• :white_check_mark: login: nirev / name: Guilherme de Maio (0886ddf3e811ed98e18383562637b6e6daf7e8a3, f86e42235d592e0e35be845ff2e0060a2c33f820, 8615321eec75bbc10b14630a06c78046ef8cadf5, 416a09e7c3d0ece16f08043593b1d1167d870cbe)

[ricardoccpaiva]

doesn't this go against span names guidelines?

[tsloughter]

It doesn't break semantic conventions since those say it should be the "route" and if that isn't available then the method.

But this should be a function, not a static name. The function should take the path and return the span name.That way it can be used to convert a path to a route.

I suppose it could accept either a string or a function, that way if you are using the client for only one path you can just set the span name to a static value.

[nirev]

@ricardoccpaiva

My understanding of the semantic convention is that providing a way to override would be fine. The only requirement is that the span name should be a low cardinality name that represents the request

Here's the relevant part from the guidelines:

HTTP spans MUST follow the overall guidelines for span names. Many REST APIs encode parameters into URI path, e.g. /api/users/123 where 123 is a user id, which creates high cardinality value space not suitable for span names. In case of HTTP servers, these endpoints are often mapped by the server frameworks to more concise HTTP routes, e.g. /api/users/{user_id}, which are recommended as the low cardinality span names. However, the same approach usually does not work for HTTP client spans, especially when instrumentation is provided by a lower-level middleware that is not aware of the specifics of how the URIs are formed. Therefore, HTTP client spans SHOULD be using conservative, low cardinality names formed from the available parameters of an HTTP request, such as "HTTP {METHOD_NAME}". Instrumentation MUST NOT default to using URI path as span name, but MAY provide hooks to allow custom logic to override the default span name.

[nirev]

@tsloughter @ricardoccpaiva

Instead of a function that takes the path, what if we allow either a string or a function that takes the Tesla.Env?

@type span_name_opt :: String.t() | (Tesla.Env.t() -> String.t())

Just added a new commit that does that. What do you think?

[fcevado]

I was waiting tesla to be merged here just to do the same type of pr. :heart:

[tsloughter]

+1 to passing the whole Env.

[ricardoccpaiva]

@tsloughter @ricardoccpaiva

Instead of a function that takes the path, what if we allow either a string or a function that takes the Tesla.Env?

@type span_name_opt :: String.t() | (Tesla.Env.t() -> String.t())

Just added a new commit that does that. What do you think?

LGTM

[nirev]

Alright. Just merged latest main branch. Should be good to go!

Thank you!

[nirev]

Hey, @tsloughter @ricardoccpaiva Do you need anything else to merge this one?

[tsloughter]

I think it is good to go. @ricardoccpaiva ?

[ricardoccpaiva]

Hey, @tsloughter @ricardoccpaiva Do you need anything else to merge this one?

✅

[tsloughter]

CI issues look to be completely unrelated to this PR, so I'll go ahead and merge.

[ricardoccpaiva]

not sure of what's the procedure not that this has been merged. Should a minor version be released?

[tsloughter]

@ricardoccpaiva yea, that'd be good.

[ricardoccpaiva]

@ricardoccpaiva yea, that'd be good.

I can take care of that but not sure if I have permissions given that the package ownership has been transfered.

[tsloughter]

@ricardoccpaiva oh, i thought we got that fixed. If not let me know and either we'll get it fixedoro I'll publish.

[ricardoccpaiva]

@tsloughter can you approve this and then after merging I'll try to release a new version of the package?

[tsloughter]

@ricardoccpaiva yup, done