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

resource.WithoutBuiltin() is ineffective if used by itself within resource.New() #1813

Closed
RichieSams opened this issue Apr 14, 2021 · 2 comments · Fixed by #1810
Closed

resource.WithoutBuiltin() is ineffective if used by itself within resource.New() #1813

RichieSams opened this issue Apr 14, 2021 · 2 comments · Fixed by #1810
Labels
bug Something isn't working
Milestone

Comments

@RichieSams
Copy link

Description

resource.WithoutBuiltin() is ineffective if used by itself within resource.New()
resource.New() will return nil, which downstream code treats as "no config, give me the defaults".

For example, prometheus.InstallNewPipeline(), will in turn call controller.New(). If the resource passed to controller.New() is nil, then controller.New() will assume that you want the defaults. AKA, all the things we wanted to remove with resource.WithoutBuiltin()

Environment

  • OS: Linux (but shouldn't matter)
  • Architecture: x64 (but shouldn't matter)
  • Go Version: 1.16
  • opentelemetry-go version: v0.19.0

Steps To Reproduce

Example code:

res, err := resource.New(
	context.Background(),
	resource.WithoutBuiltin(),
)
if err != nil {
	return nil, fmt.Errorf("Failed to initialize Prometheus exporter - %w", err)
}

// Create the metrics route
metricsHandler, err := prometheus.InstallNewPipeline(prometheus.Config{}, controller.WithResource(res))
if err != nil {
	return nil, fmt.Errorf("Failed to initialize Prometheus exporter - %w", err)
}

All prometheus metrics created will have all the built-in labels added. (Service, sdk, etc.)

Workaround

type NullDetector struct{}

// Detect implements Detector
func (NullDetector) Detect(ctx context.Context) (*resource.Resource, error) {
	return nil, nil
}
res, err := resource.New(
	context.Background(),
	resource.WithoutBuiltin(),
	resource.WithDetectors(&telemetry.NullDetector{}),
)
if err != nil {
	return nil, fmt.Errorf("Failed to initialize Prometheus exporter - %w", err)
}

// Create the metrics route
metricsHandler, err := prometheus.InstallNewPipeline(prometheus.Config{}, controller.WithResource(res))
if err != nil {
	return nil, fmt.Errorf("Failed to initialize Prometheus exporter - %w", err)
}

Expected behavior

Passing resource.WithoutBuiltin() to resource.New(), should result in no built-in labels being added.

Possible solution

resource.WithoutBuiltin() should both nil out all the built-in detectors, but also add a NullDetector, so that we're guaranteed to have a single "valid" detector on return.

@RichieSams RichieSams added the bug Something isn't working label Apr 14, 2021
@Aneurysm9
Copy link
Member

I think this might be addressed by #1810, can you take a look and see if that would meet your needs?

@RichieSams
Copy link
Author

Oh great. That's exactly the solve.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants