re-use logic for http client configuration

[zeitlinger]

doesn't cover add libraries yet - but is already usable as is

Trimmed down version of https://github.com/open-telemetry/opentelemetry-java-instrumentation/pull/11605

[zeitlinger]

@trask thanks for the review - nothing I feel strongly about, so I'm fine with adjusting if you feel it makes it clearer

[trask]

@trask thanks for the review - nothing I feel strongly about, so I'm fine with adjusting if you feel it makes it clearer

let's try to land this as a nice proposal on its own to eliminate the error prone copy-paste and deal with future concerns in follow-up

[zeitlinger]

@trask thanks for the review - nothing I feel strongly about, so I'm fine with adjusting if you feel it makes it clearer

let's try to land this as a nice proposal on its own to eliminate the error prone copy-paste and deal with future concerns in follow-up

ok, changed accordingly

[trask]

I tried out a couple of renames that helped a bit I think: https://github.com/zeitlinger/opentelemetry-java-instrumentation/pull/3

[laurit]

This PR introduces breaking changes to library instrumentations (code compiled against old library instrumentation will fail with new version). Is this ok?

[trask]

This PR introduces breaking changes to library instrumentations (code compiled against old library instrumentation will fail with new version). Is this ok?

is this because we moved the methods up to super class? (i.e. it's a source compatible change but not a binary compatible change?)

I did want to at least discuss the option of interface (with delegation) instead of abstract base class (as @breedx-splk has this always in my mind now)

[zeitlinger]

This PR introduces breaking changes to library instrumentations (code compiled against old library instrumentation will fail with new version). Is this ok?

is this because we moved the methods up to super class? (i.e. it's a source compatible change but not a binary compatible change?)

yes

I did want to at least discuss the option of interface (with delegation) instead of abstract base class (as @breedx-splk has this always in my mind now)

I thought about it. I don't like the big amount of copy-paste that this means - but it would work.

Lombok can automate that, but I think wo don't use it: https://projectlombok.org/features/experimental/Delegate

[laurit]

I did want to at least discuss the option of interface (with delegation) instead of abstract base class (as @breedx-splk has this always in my mind now)

Delegation would allow handling telemetry builders that configure both a http server and client like https://github.com/open-telemetry/opentelemetry-java-instrumentation/blob/main/instrumentation/armeria/armeria-1.3/library/src/main/java/io/opentelemetry/instrumentation/armeria/v1_3/ArmeriaTelemetryBuilder.java

[zeitlinger]

I did want to at least discuss the option of interface (with delegation) instead of abstract base class (as @breedx-splk has this always in my mind now)

Delegation would allow handling telemetry builders that configure both a http server and client like https://github.com/open-telemetry/opentelemetry-java-instrumentation/blob/main/instrumentation/armeria/armeria-1.3/library/src/main/java/io/opentelemetry/instrumentation/armeria/v1_3/ArmeriaTelemetryBuilder.java

unfortunately not, because some properties have different names, e.g. setClientSpanNameExtractor vs setSpanNameExtractor

[zeitlinger]

changed to delegation as discussed - wasn't too bad actually :smile:

[zeitlinger]

I think with the two suggestions below, then there will be no new public API and this will be easy to merge, and will be easier to review the follow-up changes

@trask done