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.

Sign up for GitHub

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

Jump to bottom

Support adding custom resource.Detectors / resource.Options #12

Closed
dstrelau opened this issue Feb 22, 2023 · 3 comments · Fixed by #47 or #48
Closed

Support adding custom resource.Detectors / resource.Options #12

dstrelau opened this issue Feb 22, 2023 · 3 comments · Fixed by #47 or #48
Assignees
@robbkidd
Labels
type: enhancement New feature or request

Comments

@dstrelau
Copy link
Member

I'd like to be able to add custom Detectors, like the ones in sdk/resource or the ones in contrib for cloud providers.

There are two problems here:

  1. I think the fix is something like adding func WithResourceOptions(...resource.Option) Option and then passing those to the resource.New call here (and sdk/resource.WithDetectors returns a resource.Option so that covers the contrib Detectors too). However, the error returned from resource.New is ignored right now and I know at least some of the AWS Detectors will return errors when not on an appropriate platform (eg, the EKS detector returns and error when not running on EKS). I'm not sure if it's worth threading that error all the way back up to the caller of ConfigureOpenTelemetry or if there are other options I'm not seeing. I'd like some advice before attempting a fix.

  2. The semconv versions imported in this project (v1.10.0) and in those other libraries (v1.17.0) are different, meaning that the semconv.SchemaURL values are different, meaning that any attempt to merge them together will fail. Happy to submit a PR to upgrade this package's version if that sounds like a reasonable fix.
@dstrelau dstrelau added the type: enhancement New feature or request label Feb 22, 2023
@pkanal pkanal added the status: oncall Flagged for awareness from Honeycomb Telemetry Oncall label Feb 27, 2023
@MikeGoldsmith
Copy link
Contributor

Hey @dstrelau - sorry for the delay in responding.

I don't see a reason why we can't upgrade to semconv v1.17 to match the other deps. Please do submit a PR to fix 👍

Regarding the supporting additional Resource Detectors, I think your idea of extending the API to include something like WithResourceOptions makes sense. Any error configuring the SDK should bubble up to the ConfigureOpenTelemetry call so the user has the option of failing or logging. We'd be happy for you to have a go at extending the API, alternatively, we'll add this to our backlog to extend when we next schedule some maintenance on the launcher.

@MikeGoldsmith MikeGoldsmith removed the status: oncall Flagged for awareness from Honeycomb Telemetry Oncall label Mar 20, 2023
@MikeGoldsmith
Copy link
Contributor

Created this ticket to update semconv separatley:

@pkanal pkanal mentioned this issue Mar 23, 2023
Merged
JamieDanielson pushed a commit that referenced this issue Mar 27, 2023
@pkanal
maint: bump semconv to 1.18 (#19)
6176970 
## Which problem is this PR solving?
Bumps the semconv version to 1.18 to match other upstream packages.

- Partially addresses
#12

## Short description of the changes
- Bumps semconv version to 1.18
- Add gitignore to ignore smoke test artifacts

## How to verify that this has the expected result
- Run smoke tests and tests
- Replace launcher dependency in the go distro with this local change to
make sure the distro works as expected
@martin308 martin308 mentioned this issue May 31, 2023
Merged
MikeGoldsmith pushed a commit that referenced this issue Jun 1, 2023
@martin308
feat: Add resource option configuration (#47)
6365f2d 
## 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
robbkidd pushed a commit that referenced this issue Jun 2, 2023
@martin308 @robbkidd
feat: Add resource option configuration (#47)
6e9cda6 
## 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
@robbkidd
Copy link
Member

robbkidd commented Jun 2, 2023

Had to revert #47 for reasons (#49), but the implementation of the feature is getting an revisit in #48.

@robbkidd robbkidd reopened this Jun 2, 2023
@robbkidd robbkidd mentioned this issue Jun 2, 2023
Merged
2 tasks
vreynolds added a commit that referenced this issue Jul 27, 2023
@robbkidd @martin308 @vreynolds
feat: Add WithResourceOption for additional resource configuration (#48)
f4716a6 
## 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 <me@martinholman.co.nz>
Co-authored-by: Vera Reynolds <verareynolds@honeycomb.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@robbkidd robbkidd

Labels
type: enhancement New feature or request
Projects
None yet
Milestone
No milestone
4 participants
@dstrelau @robbkidd @MikeGoldsmith @pkanal