Skip to content

Commit

Permalink
feat: return errors from resource.New (#59)
Browse files Browse the repository at this point in the history
## 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]>
  • Loading branch information
dstrelau and vreynolds authored Aug 14, 2023
1 parent c7780d4 commit 7129f53
Show file tree
Hide file tree
Showing 2 changed files with 87 additions and 59 deletions.
29 changes: 15 additions & 14 deletions otelconfig/otelconfig.go
Original file line number Diff line number Diff line change
Expand Up @@ -331,7 +331,7 @@ type Config struct {
errorHandler otel.ErrorHandler
}

func newConfig(opts ...Option) *Config {
func newConfig(opts ...Option) (*Config, error) {
c := &Config{
Headers: map[string]string{},
TracesHeaders: map[string]string{},
Expand All @@ -344,6 +344,8 @@ func newConfig(opts ...Option) *Config {
envError := envconfig.Process(context.Background(), c)
if envError != nil {
c.Logger.Fatalf("environment error: %v", envError)
// if our logger implementation doesn't os.Exit, we want to return here
return nil, fmt.Errorf("environment error: %w", envError)
}
// if exporter endpoint is not set using an env var, use the configured default
if c.ExporterEndpoint == "" {
Expand All @@ -366,8 +368,9 @@ func newConfig(opts ...Option) *Config {
l.logLevel = c.LogLevel
}

c.Resource = newResource(c)
return c
var err error
c.Resource, err = newResource(c)
return c, err
}

// OtelConfig is the object we're here for; it implements the initialization of Open Telemetry.
Expand All @@ -376,7 +379,7 @@ type OtelConfig struct {
shutdownFuncs []func() error
}

func newResource(c *Config) *resource.Resource {
func newResource(c *Config) (*resource.Resource, error) {
r := resource.Environment()

hostnameSet := false
Expand Down Expand Up @@ -427,19 +430,14 @@ func newResource(c *Config) *resource.Resource {

options := append(baseOptions, c.ResourceOptions...)

r, err := resource.New(
// Note: There are new detectors we may wish to take advantage
// of, now available in the default SDK (e.g., WithProcess(),
// WithOSType(), ...).
return resource.New(
context.Background(),
options...,
)

if err != nil {
c.Logger.Debugf("error applying resource options: %s", err)
}

// Note: There are new detectors we may wish to take advantage
// of, now available in the default SDK (e.g., WithProcess(),
// WithOSType(), ...).
return r
}

type setupFunc func(*Config) (func() error, error)
Expand Down Expand Up @@ -616,7 +614,10 @@ func setupMetrics(c *Config) (func() error, error) {
// ConfigureOpenTelemetry is a function that be called with zero or more options.
// Options can be the basic ones above, or provided by individual vendors.
func ConfigureOpenTelemetry(opts ...Option) (func(), error) {
c := newConfig(opts...)
c, err := newConfig(opts...)
if err != nil {
return nil, err
}

if c.LogLevel == "debug" {
c.Logger.Debugf("debug logging enabled")
Expand Down
Loading

0 comments on commit 7129f53

Please sign in to comment.