Extend client and server middleware builder to allow filtering out attributes added by default

[ioleo]

Currently there is an easy way to extend the default middleware to add additional attributes, but there is no way to disable/filter out attributes added by default implementation.

[NthPortal]

I have very mixed feelings about this.

unfortunately, this middleware started out as a port of a much older middleware that was substantially out-of-date with regard to otel semantic conventions; that said, I'd like to move it in the direction of complying with otel semantic conventions by default and to the greatest degree possible.

while on the surface this PR seems reasonable, I find myself wondering why we should be supporting things that are not part of the open telemetry semantic conventions. this includes the current customisation of the (by default convention-non-compliant) span names (which need to be fixed), but also includes filtering out arbitrary attributes. the most customisation that feels reasonable to me to support is specifying the minimum requirement level of attributes created by the middleware (required, recommended, optional(, experimental)).

in general, I feel that these middlewares should become more opinionated (primarily based on the otel semantic conventions), not the inverse. I'm curious to hear others' thoughts though; cc @iRevive @rossabaker

[NthPortal]

@ioleo what's your particular use case?

[ioleo]

@NthPortal My use case was to migrate towards otel4s library, but keep existing tag names (we're using datadog conventions currently).

I could easily add new attributes, but I did not want to push unnecessary (unused by us) attributes, and so this proposal was born.

However, I have since simply created a custom implementation for that (not using the default middleware), so if you feel like this is not a good addition to the library, lets just close it.

Eventually we plan to move towards OTEL attribute naming convetions, so in time we will probably just switch to the default impl anyways.

Thanks for your hard work on this library 🙏

I'm currently waiting for the 0.9.0 stable release - is there anything in particular missing? Can I help somehow?

[NthPortal]

I'm currently waiting for the 0.9.0 stable release - is there anything in particular missing?

@ioleo 0.9.0 stable is waiting for anyone to use the RC (such as my employer) and say it's working fine. I don't expect any issues, but I feel weird making a non-RC release without having it tested in some sort of "integration". the only reason it's even at rc.2 is because I double-bumped something in the build.sbt by accident and used up the rc.1 tag on a commit that didn't build. I fully expect the proper release to be the same commit as rc.2, so feel free to use it already if you need the newer otel4s version.

to migrate towards otel4s library, but keep existing tag names (we're using datadog conventions currently)

hmm, we definitely wouldn't want to discourage adoption for this reason. I will think more on this over the weekend

[iRevive]

I have very mixed feelings about this.

unfortunately, this middleware started out as a port of a much older middleware that was substantially out-of-date with regard to otel semantic conventions; that said, I'd like to move it in the direction of complying with otel semantic conventions by default and to the greatest degree possible.

while on the surface this PR seems reasonable, I find myself wondering why we should be supporting things that are not part of the open telemetry semantic conventions. this includes the current customisation of the (by default convention-non-compliant) span names (which need to be fixed), but also includes filtering out arbitrary attributes. the most customisation that feels reasonable to me to support is specifying the minimum requirement level of attributes created by the middleware (required, recommended, optional(, experimental)).

in general, I feel that these middlewares should become more opinionated (primarily based on the otel semantic conventions), not the inverse. I'm curious to hear others' thoughts though; cc @iRevive @rossabaker

There are a lot of good points and I agree with everything.

From what I see, there are two big goals:

1. Adjust MerticsOps in http4s so it can provide enough data to fill out all necessary attributes (see MetricsOps: use PreludeRequest and PreludeResponse http4s#7491)
2. Keep http4s-otel4s-middleware as close to the specification as possible

At the same time, I'm okay with adding some ad-hoc filters to enable/disable specific attributes. High cardinality metrics can be an issue (primarily financial).

the most customisation that feels reasonable to me to support is specifying the minimum requirement level of attributes created by the middleware (required, recommended, optional(, experimental)).

I think it's a great compromise that should be suitable for the majority of the users.

[NthPortal]

I'm wondering if it would make sense to have a trait (let's say AttributeProvider for example, bikeshedding welcome) that provides the span name, request Attributes and response Attributes, and replace the existing methods to add additional attributes with those. that way, that trait can be used to provide a default otel-compliant implementation while still allowing the use of other specifications instead, without bloating the API too much. Additionally, this could potentially allow the removal of the vault key for adding additional attributes—which has always felt a bit duplicative to me—while still making that functionality accessible to the few users who want it.

I realise I've described the tracing implementation and not one for metrics, but I assume we can do something similar for metrics without the span name.

thoughts @ioleo and @iRevive?

[iRevive]

@NthPortal sounds interesting. I guess we can give the AttributeProvider a try.

[NthPortal]

@ioleo just want to leave an update that I am actively working on implementing AttributeProvider and it's looking good so far (and also leaves the code a bit less cluttered IMO). additionally, I have been truing up the default implementation with the otel semantic conventions, so that will be another benefit (but also it's taking slightly longer to implement)

[NthPortal]

Please see #158, sorry it took so long. had to true up the implementation with semconv as well, and add tests for that