Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unclear merge order for Resources from environment variable #1620

Open
Aneurysm9 opened this issue Apr 16, 2021 · 5 comments
Open

Unclear merge order for Resources from environment variable #1620

Aneurysm9 opened this issue Apr 16, 2021 · 5 comments
Assignees
Labels
area:sdk Related to the SDK release:allowed-for-ga Editorial changes that can still be added before GA since they don't require action by SIGs spec:resource Related to the specification/resource directory

Comments

@Aneurysm9
Copy link
Member

It is unclear from a reading of the specification regarding resource information from environment variables the order that those detected resource attributes should be merged with resource attributes provided by the application. The spec states:

The SDK MUST extract information from the OTEL_RESOURCE_ATTRIBUTES environment variable and merge this, as the secondary resource, with any resource information provided by the user, i.e. the user provided resource information has higher priority.

The trouble I'm having is determining what "user provided resource information has higher priority". I would expect that the resource attributes detected from the environment, as late-binding data, would take precedence as it gives the operator-user the opportunity to override defaults provided by the devloper-user in the source code. Even in the case of run-time resource detection, allowing the environment-specified resource attributes to take precedence allows operator-users to override faulty or unwanted resource attributes coming from those detectors that they may not be able to alter via configuration.

That said, others in the Go SIG have interpreted this passage to require that resource attributes from the environment must be subordinated to any resource attributes provided by the developer-user through explicit resource construction or the use of detectors. I can see how the language currently in the specification can be interpreted in either way and suspect that this latter interpretation is what is intended.

Clarification of the language would be appreciated and, if this latter interpretation is intended, perhaps allowing for an operator-user to override this ordering through an additional environment flag could be considered. It seems like a significant reduction in flexibility compared to other environment-based configurations, such as exporter targets, that are intended to override defaults provided in code.

@Aneurysm9 Aneurysm9 added the spec:resource Related to the specification/resource directory label Apr 16, 2021
@Oberon00 Oberon00 added area:sdk Related to the SDK bug Something isn't working and removed bug Something isn't working labels Apr 20, 2021
@Oberon00
Copy link
Member

Oberon00 commented Apr 20, 2021

That said, others in the Go SIG have interpreted this passage to require that resource attributes from the environment must be subordinated to any resource attributes provided by the developer-user through explicit resource construction or the use of detectors. I can see how the language currently in the specification can be interpreted in either way

I think the language is pretty clear on that part. If you have a conflict between A and B and B has higher priority then B wins. So the user-provided resources override the environment variable. However, the question is then: (1) Does the environment variable not count as user-provided? (2) What does user-provided mean? Hardcoded?

@Aneurysm9
Copy link
Member Author

However, the question is then: (1) Does the environment variable not count as user-provided? (2) What does user-provided mean? Hardcoded?

Yeah, I think this is the crux of my issue. It seems odd to not consider environment variables as user-provided and very odd to not allow their use for overriding hardcoded values.

@carlosalberto
Copy link
Contributor

Assigning this to @Aneurysm9 in order to start gathering scenarios for this (and potentially have @jsuereth help with the write-up portion ;) )

@jsuereth
Copy link
Contributor

I'm in. For context, we had questions in general around resource detection ordering too: GoogleCloudPlatform/opentelemetry-operations-java#92

@reyang reyang added the release:allowed-for-ga Editorial changes that can still be added before GA since they don't require action by SIGs label Apr 23, 2021
@Aneurysm9
Copy link
Member Author

These use cases assume the application developer has constructed a non-default resource and that SDK implementations merge application-developer provided resources with resources detected from environment variables at the time that a TracerProvider or MeterProvider is initialized. The question is in what order should the resources be merged? Assuming arguments to resource.Merge() are in ascending priority order - that is, values from later arguments will override values from earlier arguments with the same key - which of the following is correct?

finalResource := resource.Merge(resource.Environment(), appResource)

Or

finalResource := resource.Merge(appResource, resource.Environment())

In the following use cases I would expect the latter to be used. The resource attributes from the environment should be late-binding and take precedence over any resource attributes specified in the application at build time or even attributes detected by a detector configured at application build time.

  • Specifying service name at runtime
    • Application constructs a resource with a static service name
    • Operator wishes to deploy multiple different configurations of the application and have each report a distinct service name
  • Rectifying an incorrectly detected resource
    • Application constructs a resource with one or more detectors included
    • A detector sets a resource attribute to a value an operator feels is incorrect
    • Operator wishes to use the environment to assign a different value to that attribute

These two use cases seem to cover a broad range of situations where an operator might have better information about the entity producing telemetry than application or detector developers who may be far removed from the entity’s runtime environment.

vreynolds added a commit to honeycombio/otel-config-go that referenced this issue Jul 27, 2023
“user provided resource information” the user could mean the application code writer OR the operator setting environment variables. It’s ambiguous! And not settled in the spec: open-telemetry/opentelemetry-specification#1620

See note in that issue: others in the Go SIG have interpreted this passage to require that resource attributes from the environment must be subordinated to any resource attributes provided by the developer-user

This is how the underlying Go SDK works, as designed.

The CODE>ENV decision was also made in the Ruby SDK: https://github.com/robertlaurin/opentelemetry-ruby/blob/0c03e4aee09093ce0d80142e13a35326aad9f877/sdk/test/opentelemetry/sdk/configurator_test.rb#L35
vreynolds added a commit to honeycombio/otel-config-go that referenced this issue Jul 27, 2023
## Which problem is this PR solving?

We are unable to add resource detectors when configuring the launcher.

- Closes #12

## Short description of the changes

Implementation starts with what was written for #47 with smoke tests
added to verify the order of precedence for resources attributes from
environment over code.

[vera] upon further inspection, the original behavior of CODE attributes
winning over ENV attributes was valid:

> The SDK MUST extract information from the OTEL_RESOURCE_ATTRIBUTES
environment variable and
[merge](https://opentelemetry.io/docs/specs/otel/resource/sdk/#merge)
this, as the secondary resource, with any resource information provided
by the user, i.e. the user provided resource information has higher
priority.
— [OTel Resource SDK
spec](https://opentelemetry.io/docs/specs/otel/resource/sdk/#specifying-resource-information-via-an-environment-variable)

“user provided resource information” -- the user could mean the
application code writer OR the operator setting environment variables.
It’s ambiguous! And *not* settled in the spec:
open-telemetry/opentelemetry-specification#1620

See note in that issue: 
>others in the Go SIG have interpreted this passage to require that
resource attributes from the environment must be subordinated to any
resource attributes provided by the developer-user

This is how the underlying Go SDK works, as designed.

The CODE>ENV decision was also made in the Ruby SDK:
https://github.com/robertlaurin/opentelemetry-ruby/blob/0c03e4aee09093ce0d80142e13a35326aad9f877/sdk/test/opentelemetry/sdk/configurator_test.rb#L35


Description from original PR:

Rather than adding a specific `WithDetectors` func to mirror the
functionality in the [resource
package](https://pkg.go.dev/go.opentelemetry.io/otel/sdk/resource#WithDetectors)
this adds a more generic version so that any resource option
configuration can be provided.

This has been done in a backwards compatible manner but I could see a
future where the `WithResourceAttributes` function is removed as it is
duplicate functionality now.

## How to verify that this has the expected result

- [x] Unit tests - logic's probably sound
- [x] Smoke tests - interaction with config from environment behaves
according to spec

---------

Co-authored-by: Martin Holman <[email protected]>
Co-authored-by: Vera Reynolds <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:sdk Related to the SDK release:allowed-for-ga Editorial changes that can still be added before GA since they don't require action by SIGs spec:resource Related to the specification/resource directory
Projects
None yet
Development

No branches or pull requests

5 participants