Add confighttp.WithOtelHTTPOptions

[yurishkuro]

Description

Add ability to provide OTel HTTP intrumentation options to ToServer() function.

Link to tracking issue

Fixes #11770
Related to jaegertracing/jaeger#6265

Testing

Added unit test

[linux-foundation-easycla]

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.58%. Comparing base (751768f) to head (34127ed).
Report is 1 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main   #11769   +/-   ##
=======================================
  Coverage   91.58%   91.58%           
=======================================
  Files         446      448    +2     
  Lines       23741    23755   +14     
=======================================
+ Hits        21743    21757   +14     
  Misses       1623     1623           
  Partials      375      375           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[mx-psi]

otelhttp is not 1.0 and has no plans to be 1.0 in the foreseeable future. Introducing this as part of the public API of confighttp would prevent us from marking it as 1.0 until otelhttp is marked as 1.0.

Is there a way to introduce this so that WithOtelHTTPOptions either does not depend on a symbol in otelhttp or is not in the confighttp package?

[yurishkuro]

Introducing this as part of the public API of confighttp would prevent us from marking it as 1.0 until otelhttp is marked as 1.0.

• The chance that otelhttp would change the "options" concept is near zero.
• If they do, we can always deprecate the new option here or make it no-op.
• This module depends on plenty of other 0.x modules, both internal and external e.g. github.com/golang/snappy v0.0.4

[yurishkuro]

/easycla

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: yurishkuro / name: Yuri Shkuro (e737b57, a1a33f6, c6a0538, 4ad8f49, ae820f8, d4f6608, b3f25fe, 7187d43, 6c7b231, 9aab2cf, 7da40b7, 4eebef0, 22e57e5, dc195a5, 34127ed, f093567, e94a48c)

[yurishkuro]

/easycla

[mx-psi]

Introducing this as part of the public API of confighttp would prevent us from marking it as 1.0 until otelhttp is marked as 1.0.

• The chance that otelhttp would change the "options" concept is near zero.

Agreed

• If they do, we can always deprecate the new option here or make it no-op.

Worst case scenario is that the concept is removed altogether. I think this is extremely unlikely, but we wouldn't have a non-breaking way of dealing with it.

• This module depends on plenty of other 0.x modules, both internal and external e.g. github.com/golang/snappy v0.0.4

The particular rule we are breaking is this one from the VERSIONING.md doc:

Dependency updates. Updating dependencies is not considered a breaking change except when their types are part of the public API or the update may change the behavior of applications in an incompatible way.

I am fine accepting this change if we first amend VERSIONING.md to reflect the new policy we are following.

[yurishkuro]

Dependency updates. Updating dependencies is not considered a breaking change except when their types are part of the public API or the update may change the behavior of applications in an incompatible way.

Hm, I don't know how we can do this without exposing otelhttp.Option type. We could fully duplicate their options and translate, but that feels like a massive overkill.

[mx-psi]

One way of doing this without changing VERSIONING.md is to move this option to a separate module. It feels like an overkill to have a separate module for just one option but I guess modules are free 🤷 This is basically the pattern we have here: https://pkg.go.dev/go.opentelemetry.io/collector/pipeline/pipelineprofiles

[yurishkuro]

to move this option to a separate module

don't think it will work, the options operate on a private struct in confighttp

[mx-psi]

to move this option to a separate module

don't think it will work, the options operate on a private struct in confighttp

The way to do this is to move the struct to an internal package, define all options in that internal package and re-export them in the appropriate module (the otelhttp one in a separate module, the others in confighttp)

[yurishkuro]

I don't follow that. Internal package would have to be in some module, so another module would not be able to reference it.

But if we do it in the same module with an experimental sub-package then it could work, e.g confighttp.confighttpx.WithOtelHTTPOptions

[mx-psi]

@yurishkuro Here is a more explicit explanation of the pattern: https://dmathieu.com/en/development/go-architecture-semver/stable-experimental-modules/ I think we are talking about the same thing

[yurishkuro]

@mx-psi I move it to new experimental module, lmk if any concerns

[yurishkuro]

/easycla

[mx-psi]

This looks good to me (modulo solving the EasyCLA thing), but as I commented on #11779 I think we should settle in a convention for experimental module names by opening a PR here. I personally like the exp suffix better

[jarias-lfx]

/easycla

[yurishkuro]

@mx-psi #11783

[mx-psi]

Can you update the changelog to reference that this is in xcongihttp? LGTM otherwise :)