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

Add OpenCensus reader Bridge, and Prometheus Bridge option #3207

Closed
wants to merge 6 commits into from

Conversation

dashpole
Copy link
Contributor

@dashpole dashpole commented Sep 20, 2022

This is a way to unblock the collectors' use of the opencensus bridge + prometheus exporter. It adds a WithBridge option to the prometheus exporter. It only works for prometheus, and not other pull exporters.

Example usage:

// Create the opencensus bridge metric reader, and pass it to the prometheus exporter as its "source" of metrics.
collector := prometheus.NewExporter(
    prometheus.WithBridge(opencensus.NewPrometheusBridge()),
)
// use the prometheus exporter as usual:
registry := prometheus.NewRegistry()
err := registry.Register(collector)
if err != nil {
	fmt.Printf("error registering collector: %v", err)
	return
}

log.Printf("serving metrics at localhost:2222/metrics")
http.Handle("/metrics", promhttp.HandlerFor(registry, promhttp.HandlerOpts{}))
err = http.ListenAndServe(":2222", nil)
if err != nil {
	fmt.Printf("error serving http: %v", err)
	return
}

This does not align with what is currently being discussed in the specification PR, but will solve the problem for the collector while we work on that.

@codecov
Copy link

codecov bot commented Sep 20, 2022

Codecov Report

Merging #3207 (3777f81) into main (aca054b) will increase coverage by 0.3%.
The diff coverage is 88.8%.

❗ Current head 3777f81 differs from pull request most recent head 91e170f. Consider uploading reports for the commit 91e170f to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##            main   #3207     +/-   ##
=======================================
+ Coverage   77.3%   77.6%   +0.3%     
=======================================
  Files        159     159             
  Lines      11184   11222     +38     
=======================================
+ Hits        8651    8719     +68     
+ Misses      2335    2303     -32     
- Partials     198     200      +2     
Impacted Files Coverage Δ
exporters/prometheus/exporter.go 88.1% <86.3%> (+9.6%) ⬆️
bridge/opencensus/metric.go 91.8% <91.3%> (+91.8%) ⬆️
sdk/trace/batch_span_processor.go 81.1% <0.0%> (+0.8%) ⬆️

Copy link
Member

@paivagustavo paivagustavo left a comment

Choose a reason for hiding this comment

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

This looks good! Only one small comment.

bridge/opencensus/metric.go Outdated Show resolved Hide resolved
@dashpole
Copy link
Contributor Author

Notes from the SIG meeting:

  • The spec PR seems so close! If we can get agreement soon-ish, it would be better to align with the spec, even if it isn't released.
    • @jmacd will try and work out some of the recent disagreements that cropped up (thanks!).
  • If the spec PR is still up in the air by 9/26, I'll move forward with something similar to this PR

@dashpole dashpole force-pushed the reader_bridge branch 2 times, most recently from 5bae1e9 to 251a124 Compare September 22, 2022 18:23
@dashpole
Copy link
Contributor Author

I implemented the idea I had at the end of the sig meeting, which is to have prometheus define an interface without the rest of the Reader functions. It makes it much less bad IMO.

It is now prometheus-specific, but that could be changed if we put the Bridge interface somewhere else.

// Bridge is a source of metrics other than the OpenTelemetry SDK.
type Bridge interface {
// Collect gathers and returns all metric data from the Bridge.
Collect(context.Context) (metricdata.ScopeMetrics, error)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note that this provides a ScopeMetrics, rather than a ResourceMetrics. This ensures we only get the resource from the SDK.

@dashpole dashpole changed the title [PoC] Add OpenCensus reader bridge [PoC] Add OpenCensus reader bridge specifically for prometheus Sep 27, 2022
Copy link
Contributor

@jmacd jmacd left a comment

Choose a reason for hiding this comment

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

Looks good.

@dashpole dashpole force-pushed the reader_bridge branch 2 times, most recently from 5dd1ca1 to 977ccf6 Compare September 28, 2022 19:58
}

// ExportMetrics implements the OpenCensus metric Exporter interface by sending
// to an OpenTelemetry exporter.
func (e *exporter) ExportMetrics(ctx context.Context, ocmetrics []*ocmetricdata.Metric) error {
otelmetrics, err := internal.ConvertMetrics(ocmetrics)
if err != nil {
return err
otel.Handle(err)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was a bug I found while adding unit tests. This allows the bridge to have partial success sending metrics, and keeps the behavior of the previous OC bridge implementation.

@dashpole dashpole changed the title [PoC] Add OpenCensus reader bridge specifically for prometheus Add OpenCensus reader bridge specifically for Prometheus Sep 28, 2022
@dashpole dashpole marked this pull request as ready for review September 28, 2022 20:05
@dashpole
Copy link
Contributor Author

This is ready for review now.

@dashpole dashpole changed the title Add OpenCensus reader bridge specifically for Prometheus Add OpenCensus reader Bridge, and Prometheus Bridge option Sep 29, 2022
Copy link
Member

@paivagustavo paivagustavo left a comment

Choose a reason for hiding this comment

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

Thanks @dashpole!

As part of planning the migration of the otel collector instrumentation from OpenCensus to Otel Go, this would help us being able to have an incremental and hopefully smoother migration of the existing instrumentation in the collector core and contrib project.

Without the bridge our option would be to migrate the whole project (and contrib) at once, or migrate it incrementally to otel go losing some metrics in the first moment until everything gets migrated.

bridge/opencensus/metric.go Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants