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

config: Add WithPrometheusRegisterer config option #6091

Closed
wants to merge 4 commits into from
Closed

config: Add WithPrometheusRegisterer config option #6091

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
0f4b201
config: Add `WithRegistry`
codeboten Sep 5, 2024
6458944
Update CHANGELOG.md
codeboten Sep 5, 2024
544aee6
update with registerer instead
codeboten Sep 5, 2024
111b5b0
cleanup
codeboten Sep 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm
### Added

- Add `NewProducer` to `go.opentelemetry.io/contrib/instrumentation/runtime`, which allows collecting the `go.schedule.duration` histogram metric from the Go runtime. (#5991)
- Add `WithPrometheusRegisterer` to `go.opentelemetry.io/contrib/config` to allow users to configure their own prometheus registry and handler. (#6091)

### Removed

Expand Down
17 changes: 15 additions & 2 deletions config/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
"context"
"errors"

"github.com/prometheus/client_golang/prometheus"
"go.opentelemetry.io/otel/log"
"go.opentelemetry.io/otel/metric"
"go.opentelemetry.io/otel/trace"
Expand All @@ -21,8 +22,9 @@
)

type configOptions struct {
ctx context.Context
opentelemetryConfig OpenTelemetryConfiguration
ctx context.Context
opentelemetryConfig OpenTelemetryConfiguration
prometheusRegisterer prometheus.Registerer
}

type shutdownFunc func(context.Context) error
Expand Down Expand Up @@ -127,6 +129,17 @@
})
}

// WithPrometheusRegisterer sets a Prometheus registerer to be used by
// any Prometheus exporters configured in this SDK. Note: if this option
// is set, the caller is expected to initialize their own handler to
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you help explain this a bit more to me? Doesn't this mean most of the prometheus configuration provided via config (e.g. port, path) is ignored? Why does the collector need this behavior?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So the bulk of the prometheus handler code in the collector exists here: https://github.com/open-telemetry/opentelemetry-collector/blob/e99074da2f6525cd177f26b46b8d03fef2fe4f35/service/internal/proctelemetry/config.go#L106-L116

I'm totally open to suggestions as to a better way to implement this, the problem I was trying to solve was handling errors without passing a channel around.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we should use the otel error handler for the async errors from this package: https://github.com/open-telemetry/opentelemetry-go/blob/29c0c28a2c13f898c8245250aab8f5f336250a91/handler.go#L33. Then also handle those errors generally in the collector however you see fit? What do you do with async errors you get back?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So the error channel used terminates the collector on error:

    // AsyncErrorChannel is the channel that is used to report fatal errors.
    AsyncErrorChannel chan error

I suppose we could register an error handler and set it to return the error to the channel instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll try to implement that and see how far i can get. will put this in draft until I get that working

// expose Prometheus metrics.
func WithPrometheusRegisterer(reg prometheus.Registerer) ConfigurationOption {
return configurationOptionFunc(func(c configOptions) configOptions {
c.prometheusRegisterer = reg
return c
})

Check warning on line 140 in config/config.go

View check run for this annotation

Codecov / codecov/patch

config/config.go#L136-L140 

Added lines #L136 - L140 were not covered by tests
}

// TODO: implement parsing functionality:
// - https://github.com/open-telemetry/opentelemetry-go-contrib/issues/4373
// - https://github.com/open-telemetry/opentelemetry-go-contrib/issues/4412
Expand Down
24 changes: 16 additions & 8 deletions config/metric.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@

var errs []error
for _, reader := range cfg.opentelemetryConfig.MeterProvider.Readers {
r, err := metricReader(cfg.ctx, reader)
r, err := metricReader(cfg, reader)
if err == nil {
opts = append(opts, sdkmetric.WithReader(r))
} else {
Expand All @@ -69,7 +69,7 @@
return mp, mp.Shutdown, nil
}

func metricReader(ctx context.Context, r MetricReader) (sdkmetric.Reader, error) {
func metricReader(cfg configOptions, r MetricReader) (sdkmetric.Reader, error) {
if r.Periodic != nil && r.Pull != nil {
return nil, errors.New("must not specify multiple metric reader type")
}
Expand All @@ -83,18 +83,18 @@
if r.Periodic.Timeout != nil {
opts = append(opts, sdkmetric.WithTimeout(time.Duration(*r.Periodic.Timeout)*time.Millisecond))
}
return periodicExporter(ctx, r.Periodic.Exporter, opts...)
return periodicExporter(cfg.ctx, r.Periodic.Exporter, opts...)
}

if r.Pull != nil {
return pullReader(ctx, r.Pull.Exporter)
return pullReader(cfg, r.Pull.Exporter)
}
return nil, errors.New("no valid metric reader")
}

func pullReader(ctx context.Context, exporter MetricExporter) (sdkmetric.Reader, error) {
func pullReader(cfg configOptions, exporter MetricExporter) (sdkmetric.Reader, error) {
if exporter.Prometheus != nil {
return prometheusReader(ctx, exporter.Prometheus)
return prometheusReader(cfg, exporter.Prometheus)
}
return nil, errors.New("no valid metric exporter")
}
Expand Down Expand Up @@ -214,7 +214,7 @@
return otlpmetricgrpc.New(ctx, opts...)
}

func prometheusReader(ctx context.Context, prometheusConfig *Prometheus) (sdkmetric.Reader, error) {
func prometheusReader(cfg configOptions, prometheusConfig *Prometheus) (sdkmetric.Reader, error) {
var opts []otelprom.Option
if prometheusConfig.Host == nil {
return nil, fmt.Errorf("host must be specified")
Expand Down Expand Up @@ -248,6 +248,14 @@
}
}

// if a registry is set, the caller is expected
// to setup a prometheus server, all that's needed
// is to pass the option and return the reader here
if cfg.prometheusRegisterer != nil {
opts = append(opts, otelprom.WithRegisterer(cfg.prometheusRegisterer))
return otelprom.New(opts...)

Check warning on line 256 in config/metric.go

View check run for this annotation

Codecov / codecov/patch

config/metric.go#L255-L256 

Added lines #L255 - L256 were not covered by tests
}

reg := prometheus.NewRegistry()
opts = append(opts, otelprom.WithRegisterer(reg))

Expand All @@ -271,7 +279,7 @@
if err != nil {
return nil, errors.Join(
fmt.Errorf("binding address %s for Prometheus exporter: %w", addr, err),
reader.Shutdown(ctx),
reader.Shutdown(cfg.ctx),

Check warning on line 282 in config/metric.go

View check run for this annotation

Codecov / codecov/patch

config/metric.go#L282 

Added line #L282 was not covered by tests
)
}

Expand Down
2 changes: 1 addition & 1 deletion config/metric_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -480,7 +480,7 @@ func TestReader(t *testing.T) {
}
for _, tt := range testCases {
t.Run(tt.name, func(t *testing.T) {
got, err := metricReader(context.Background(), tt.reader)
got, err := metricReader(configOptions{}, tt.reader)
require.Equal(t, tt.wantErr, err)
if tt.wantReader == nil {
require.Nil(t, got)
Expand Down
Loading