feat: Add WithResourceOption for additional resource configuration #48
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
robbkidd
force-pushed
the
robb.smoke-resources
branch
from
23ece81 to
7856679
Compare
robbkidd
force-pushed
the
robb.smoke-resources
branch
from
7856679 to
a3c9917
Compare
robbkidd
commented
Jun 1, 2023
examples/main.go
Outdated
|
|
||
| if err != nil { | ||
| log.Fatalf("error setting up OTel SDK - %e", err) | ||
| } | ||
|
|
||
| defer otelShutdown() | ||
| tracer := otel.Tracer("my-app") | ||
| ctx := context.Background() | ||
| ctx, span := tracer.Start(ctx, "doing-things") | ||
| _, span := tracer.Start(context.Background(), "doing-things") |
There was a problem hiding this comment.
I … don't know why I included this. It has nothing to do with the resource attributes issue. 😐
robbkidd
added
the
status: oncall
## 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
Per the OTel spec, resource attributes set via environment variable must supercede resource attributes set in code.[1] [1] https://opentelemetry.io/docs/specs/otel/resource/sdk/#specifying-resource-information-via-an-environment-variable
robbkidd
force-pushed
the
robb.smoke-resources
branch
from
a3c9917 to
d23bbf7
Compare
robbkidd
changed the title
What's up with the revert/readd of this feature?Resource attributes set via environment variable must supercede resource attributes set in code.
#47 introduced |
robbkidd
added
type: enhancement
dstrelau
reviewed
Jun 5, 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
|
Updated the description with my reasoning for walking back on the premise of the revert. I believe code config wins over env config, at least in the Go SDK land. |
vreynolds
added
the
version: bump minor
vreynolds
changed the title
JamieDanielson
approved these changes
Jul 27, 2023
There was a problem hiding this comment.
It seems like we're still doing some extra work here that could be simplified by using the available resource options now available upstream, but that can be addressed separately. This at least allows the end user more flexibility in setting additional resource options. Appreciate the PR details and the test cleanup as well 👍
JamieDanielson
pushed a commit
that referenced
this pull request
Aug 14, 2023
## Which problem is this PR solving? When using detectors with mis-matched Schema URLs, resource.New will return an error. Previously, this was dropped and consequently it looked like configuration worked but many resource attributes would be missing in the resulting telemetry. With the recent addition of #48, this error is much easier to hit. For example, the detectors built into opentelemetry-go [import](https://github.com/open-telemetry/opentelemetry-go/blob/main/sdk/resource/builtin.go#L25) semconv `v1.17.0` but this project [pulls in](https://github.com/honeycombio/otel-config-go/blob/main/otelconfig/otelconfig.go#L21) `v1.18.0`. This means that adding something like `otelconfig.WithResourceOption(resource.WithHost())` results in a mis-configured OTel and no error to warn you what happened. This is all … very bad. This PR is really the bare minimum that needs to be done here. That is, it at least causes a runtime error so that you can tell what's going on — but it doesn't solve the problem that you fundamentally cannot use this library with _any_ other library that doesn't happen to use semconv `v1.18.0`. There are [a number](open-telemetry/opentelemetry-go#2341) of [open](open-telemetry/opentelemetry-go#3769) [issues](open-telemetry/opentelemetry-go-contrib#3657) in OTel and adjacent repositories regarding this problem, which probably warrant further investigation into a long-term sustainable solution. ## Short description of the changes Return the `err` from `resource.New` up through the stack. Update tests to expect this.⚠️ I guess technically this could be considered a backwards incompatible change, in that folks may have configurations that are "working" today but will throw an error after this change. I put that in scare quotes though because it's not _really_ working — it's just not throwing an error. I feel like actually returning an appropriate error here and letting folks know their configuration is likely not working as expected is the right choice. ## How to verify that this has the expected result There's a new test that specifically uses a detector with a different schema URL. 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
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:
“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:
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
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