Clean up whitespace in OTEL_RESOURCE_ATTRIBUTES

[inssein]

Fixes #1540

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: inssein / name: Hussein Jafferjee (14a4832, 09d652e, 76d88fe, 9be4469)
• ✅ login: arielvalentin / name: Ariel Valentin (3937cc2)

[mjallday]

👍

[arielvalentin]

Should this trim values as well?

Suggested change

	OpenTelemetry::TestHelpers.with_env('OTEL_RESOURCE_ATTRIBUTES' => 'important_foo=x, important_bar=y') do
	OpenTelemetry::TestHelpers.with_env('OTEL_RESOURCE_ATTRIBUTES' => ' important_foo=x, important_bar=y ') do

[inssein]

I thought about this but decided against it - I could add it in, not sure what other otel instrumentation agents do.

[arielvalentin]

I believe this should be using Baggage format so should be trimming spance then URL decoding https://github.com/open-telemetry/opentelemetry-go/blob/main/sdk/resource/env.go#L92

[inssein]

This is my first time ever writing ruby and I haven't been able to run the tests locally (got ruby 2 installed), but I have made the changes and hopefully they make sense.

[inssein]

wait sorry I messed up the order, trim first! committing in a moment

[arielvalentin]

@inssein checking in since it's been a while. Looks like the test suite is failing. Would you mind taking a look again?

[inssein]

@arielvalentin I took a quick look but I can't figure out how my test is wrong. I don't quite have a local development environment going, would love a pointer.

Edit: odd that it's only partial failures at the OS level.

[xuan-cao-swi]

I suspect it's decode_www_form_component cause the cross-system difference. Could you try to remove URI.decode_www_form_component and see if it pass all tests?

And/Or use strip! instead of strip on the string?

[robbkidd]

I'm suspecting there's also some intermittent flakiness in the tests. I started playing with the changes in this PR in my dev environment because I too could not figure out what was wrong. Tests pass most of the time, but error occasionally saying the two important_{foo|bar} attributes are missing from the expected resources.

[github-actions]

👋 This pull request has been marked as stale because it has been open with no activity. You can: comment on the issue or remove the stale label to hold stale off for a while, add the keep label to hold stale off permanently, or do nothing. If you do nothing this pull request will be closed eventually by the stale bot

[inssein]

Could I get permission to run workflows or does it have to be approved each time?

[arielvalentin]

Could I get permission to run workflows or does it have to be approved each time?

The current policy is only for first time contributors. Once your first PR is merged, the policy will allow actions to run automatically for future PRs.

Thanks again for your understanding and your first contribution.

[inssein]

When I run these tests locally via docker I get no failures:

opentelemetry-ruby git:(otel-resource-attributes-env) ✗ docker-compose run sdk bundle exec rake test
[+] Building 0.0s (0/0)                                                                                                                                                    docker:default
[+] Building 0.0s (0/0)                                                                                                                                                    docker:default
/app/sdk/test/opentelemetry/sdk/trace/span_test.rb:469: warning: assigned but unused variable - yielded_span
/app/sdk/test/opentelemetry/sdk/trace/tracer_test.rb:11: warning: already initialized constant SpanLimits
/app/sdk/test/opentelemetry/sdk/trace/span_test.rb:14: warning: previous definition of SpanLimits was here
Run options: --seed 37135

# Running:

..................................................................................................................................................................................................................................................................................

Finished in 2.043577s, 134.0786 runs/s, 238.7970 assertions/s.

274 runs, 488 assertions, 0 failures, 0 errors, 0 skips
Coverage report generated for Integration Tests to /app/sdk/coverage. 2494 / 2536 LOC (98.34%) covered.

I'm not a ruby developer and I don't quite understand what's going on. I can remove the uri decoding stuff but then it wouldn't be to spec.

[github-actions]

👋 This pull request has been marked as stale because it has been open with no activity. You can: comment on the issue or remove the stale label to hold stale off for a while, add the keep label to hold stale off permanently, or do nothing. If you do nothing this pull request will be closed eventually by the stale bot

[arielvalentin]

I think I understand why this is failing. The OTEL_RESOURCE_ATTRIBUTES are computed by a singleton Resources.default().

This test is likely running in a non-deterministic order and if the test runs first, the singleton is set with the appropriate values, however if another test sets the singleton then this test fails because it will not re-evaluate the environment value.

Instead of making this test part of the SDK configurator like you have it in here, I recommend using the Resource related tests for default, which is currently resetting the singleton value:

opentelemetry-ruby/sdk/test/opentelemetry/sdk/resources/resource_test.rb

Line 44 in 35abeac

describe '.default' do

This will make your test suite stable and predictable while ensuring correctness.