feat: Add resource option configuration #47
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
martin308
changed the title
dstrelau
approved these changes
Jun 1, 2023
MikeGoldsmith
approved these changes
Jun 1, 2023
MikeGoldsmith
approved these changes
Jun 1, 2023
2 tasks
robbkidd
added a commit
that referenced
this pull request
Jun 2, 2023
This reverts commit 6365f2d. This implementation of adding resource attributes with code clobbers resource attributes set from environment variables which is against spec.
robbkidd
added a commit
that referenced
this pull request
Jun 2, 2023
## Which problem is this PR solving? This implementation of adding resource attributes with code clobbers resource attributes set from environment variables which is against spec. ## Short description of the changes This reverts commit 6365f2d.
robbkidd
pushed a commit
that referenced
this pull request
Jun 2, 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 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 Unit tests have been added
vreynolds
added a commit
that referenced
this pull request
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
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Which problem is this PR solving?
We are unable to add resource detectors when configuring the launcher.
Short description of the changes
Rather than adding a specific
WithDetectorsfunc to mirror the functionality in the resource package 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
WithResourceAttributesfunction is removed as it is duplicate functionality now.How to verify that this has the expected result
Unit tests have been added