[cowboy instrumenter] Allow users to set custom attributes on span start

[albertored]

Maybe better to get this configuration parameter from the setup function instead of application env but for now this PR it's a draft waiting for comments.

The use case is to be able to filter spans with a sampler based on user attributes that, if added after span creation, are not present when the sampling decision is taken.

See also discussion on slack

[tsloughter]

I think this makes sense, just needs to, as you mention, not be read from the environment on every request but once in setup.

[albertored]

@tsloughter done. Not sure about the options type, I'm using a keyword at the moment (as I would have done in Elixir to have a better typespec), not sure how it is usually done in Erlang

[tsloughter]

Keyword list v map is one of the more annoying interop issues we've run in to in OpenTelemetry -- other main one being nil v undefined. A keyword list is still not out of the ordinary in Erlang but maps have started to take their place.

I guess I'd say do what is done in Cowboy for options. Is it a map in 2.0 mostly?

[albertored]

Yes, they are maps in cowboy, I'll change

[tsloughter]

So talking with a user it came up the need to do this and they mentioned Javascript instrumentation libraries take a startSpanHook that can then call set_attributes.

Made me realize we should probably not limit this to attributes but instead take a fun like, start_span_callback or _fun that runs with the Span active so they can just call Traceer.set_attribute.

[tsloughter]

Oops. @bryannaegele corrected me that for sampling the attributes need to be available at the call of start.

So maybe this is ready to merge?

[tsloughter]

We'll need a follow up that allows for this: https://github.com/open-telemetry/semantic-conventions/blob/main/docs/http/http-spans.md#http-request-and-response-headers

This fun is more generic and takes the whole req so is fine that it doesn't follow the semconv which only applies to headers.

[tsloughter]

Damn, tests fail.

[albertored]

@tsloughter upgraded deps and a failing test, let's if it works now.

[albertored]

Before merging we should choose a good name for the option, in principle it can be added also to other HTTP client/server instrumenters. In js for instance they call it requestHook and responseHook, I like it because we can use the same name and the same functionality also for every isntrumenter that has a request/response concept, not only HTTP.

[tsloughter]

Yea, I think request/response_hook is good.

[albertored]

@tsloughter ready

[albertored]

Is there something missing to this to be merged?

[bryannaegele]

Hey @albertored. I have to review it still and just got home from 2 weeks of holiday. I'll be working through my backlog over the next couple weeks

[albertored]

@bryannaegele perfect thanks! I was just wondering if there was something missing from my side