Avoid overriding explicitly set grpc dial options

[mmcshane]

The contract of oteltracegrpc.WithDialOption is that the options supplied become the tail end of the dial option slice that is eventually supplied to grpc.Dial however in some cases the implementation violates that contract and appends additional dial options at the end of said slice.

This commit changes otlpconfig option processing to ensue that the contract of oteltracegrpc.WithDialOption is upheld. That is, the options supplied become the suffix of the slice used to dial.

Fixes #2940

[linux-foundation-easycla[bot]]

The committers listed above are authorized under a signed CLA.

• :white_check_mark: login: mmcshane / name: Matt McShane (3847919b5e76d0e9b6e9fb5ec8299c04b5d15de6)

[codecov[bot]]

Codecov Report

Merging #2942 (1e9d9f9) into main (acce4e6) will increase coverage by 0.0%. The diff coverage is 100.0%.

@@          Coverage Diff          @@
##            main   #2942   +/-   ##
=====================================
  Coverage   75.7%   75.7%           
=====================================
  Files        177     177           
  Lines      11813   11813           
=====================================
+ Hits        8945    8951    +6     
+ Misses      2634    2630    -4     
+ Partials     234     232    -2     

Impacted Files	Coverage Δ
...ters/otlp/otlptrace/internal/otlpconfig/options.go	89.1% <100.0%> (+7.7%)	:arrow_up:
sdk/trace/batch_span_processor.go	80.2% <0.0%> (-1.8%)	:arrow_down:

[mmcshane]

The only question I have is does the comment of WithDialOptions still make sense after this change?

The comment was and remains sensible, the problem is that it was not correct with respect to the implementation. This patch brings the code into alignment with the comment. I.e. the code didn't do what the comment says and now (I think) it does.

[mmcshane]

Added some additional options to the unit test to address the coverage drop.

Should I squash these commits or do the maintainers do that at merge time?

[dmathieu]

No need to squash the commits. It will be done by GitHub at merge time.

[MadVikingGod]

So I don't think this patch makes things any better.

Take using these two options together:

otlptracegrpc.WithServiceConfig("foo") and otlptracegrpc.WithDialOptions(grpc.WithDefaultServiceConfig("bar"))

With the status quo if you do:

otlptracegrpc.WithServiceConfig("foo"),
otlptracegrpc.WithDialOptions(grpc.WithDefaultServiceConfig("bar")),

// You get a config with foo.

But with your changes if you do:

otlptracegrpc.WithDialOptions(grpc.WithDefaultServiceConfig("bar")),
otlptracegrpc.WithServiceConfig("foo"),

// You get a config with bar.

Both cases don't really make sense, and I don't think one is particularly better than the other. What I would rather see is that if we have compeating ways of adding the same config they play well together, so both overwrite the other. If that isn't possible then the comments need to be extra clear that using one will overwrite the other. (// This option has no effect if WithGRPCConn is used or if WithDialOptions is used.)

[mmcshane]

Rework the overall approach as and when you see fit. The intention here is to only bring the behavior in line with the existing documentation.

I'm only one datapoint but my reading of the docs resulted in an understanding that a tie-breaker for conflicting options had indeed been chosen: "The options here are appended to the internal grpc.DialOptions used so they will take precedence over any other internal grpc.DialOptions they might conflict with.". I don't really know the history of this package but that seems like a clear and intentional statement. When you say "the comments need to be extra clear that using one will overwrite the other," you're correct, but I submit that that is the case already.

I coded with the expectation that this package would work as indicated by the docs. The fact that it doesn't is what led to this PR. The details are in the bug report and the reproduction.

[MadVikingGod]

It's clear that the wording there is ambiguous. To me 'other internal grpc.DialOptions` means the defaults, or options not exposed. Where you take it to mean something that includes the other Options in that package. My ask is to change the wording to be more explicit, so users that follow after you won't deal with the ambiguity you found here.

[MrAlias]

This looks to have stalled. Please re-open if you intend to pick this back up.