cli-plugins: merge OTEL_RESOURCE_ATTRIBUTES environment variable

[jsternberg]

- What I did

Merge OTEL_RESOURCE_ATTRIBUTES when there is one already in the environment. This allows user-specified resource attributes to be passed on to CLI plugins while still allowing the extra attributes added for telemetry information.

This was the original intended use-case but it seems to have never made it in. The reason OTEL_RESOURCE_ATTRIBUTES was used is because we could combine it with user-centric ones.

Additionally, remove the docker.cli prefixed attributes from OTEL_RESOURCE_ATTRIBUTES after the telemetry provider has been created within a plugin. This prevents accidentally sending the attributes to something downstream for the user.

This also fixes an issue with compose where the self-injected OTEL_RESOURCE_ATTRIBUTES would override an existing attribute in the environment file because the "user environment" overrode the environment file, but the "user environment" was created by the docker tool rather than by the user's environment.

When OTEL_RESOURCE_ATTRIBUTES is empty after pruning, the environment
variable is unset.

Fixes #4958.

- How I did it

Check the environment variable and combine it with a comma to ensure all attributes are present in the final environment variable. The injected attributes take priority but they are namespaced and shouldn't conflict with user-specified ones.

For sanitizing the environment variable, I used the initialize method from the plugin to look for docker.cli attributes in the environment and remove them.

- How to verify it

Should be able to verify by compiling compose with the change and running this with this compose.yml.

services:
  web:
    image: alpine
    command: env && cat
    environment:
      - OTEL_RESOURCE_ATTRIBUTES

And this .env file.

OTEL_RESOURCE_ATTRIBUTES="a.b.c=foobar"

If you see a.b.c=foobar in the output it's working. You can also run the above with setting OTEL_RESOURCE_ATTRIBUTES=a.b.d=foobar docker compose up and if you see a.b.d then that shows the first part is working. When both are together, only a.b.d=foobar will be present. When only the CLI is fixed and compose is still without the change, you'll see a.b.d=foobar with the one the CLI injects.

- Human readable description for the release notes

- Merge user-specified `OTEL_RESOURCE_ATTRIBUTES` with CLI injected ones
- Remove CLI injected resource attributes after plugin initialization

- A picture of a cute animal (not mandatory but encouraged)

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 39.13043% with 28 lines in your changes missing coverage. Please review.

Project coverage is 59.22%. Comparing base (a8f8886) to head (8890a1c).
Report is 6 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #5842      +/-   ##
==========================================
+ Coverage   59.16%   59.22%   +0.05%     
==========================================
  Files         352      352              
  Lines       29567    29607      +40     
==========================================
+ Hits        17493    17534      +41     
+ Misses      11092    11087       -5     
- Partials      982      986       +4     

[Benehiko]

wouldn't b be nil on the first iteration?

Why not have a slice of members and then add it to the baggage afterwards.

var members []baggage.Member
for iter := attrs.Iter(); iter.Next(); {
  m, err := baggage.NewMemberRaw(string(attr.Key), attr.Value.AsString())
  if err != nil {
    continue
  }
  members = append(members, m)
}
b := baggage.New(members...)

[jsternberg]

Baggage operates similar to a slice so it being nil isn't really a problem. SetMember is similar to calling append on a slice. Looking at the implementation, there is a possibility there's a slight performance improvement for New if the number of baggage members are above 1, but this will only ever be 1. I can change it though since it's not a big deal.

[Benehiko]

Thank you! it was just quite difficult to read, since we were assigning to a variable in the outer-scope on every iteration - just looked strange.

for //
newB, err := b.SetMember(m)
// handle err
b = newB

[Benehiko]

LGTM

[thaJeztah]

Sorry, away from keyboard for some hours; is any of these depending on generics or otherwise that would require a "//go:build" hack?

cc @vvoland we probably should run the go module check to be safe

[thaJeztah]

(I tentatively added this to the v28.0 milestone, but wasn't sure how critical it was)

[jsternberg]

@thaJeztah no generics or any //go:build hacks.