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

Restructure instrument creation code paths #3256

Merged
merged 35 commits into from
Nov 11, 2022

Conversation

MrAlias
Copy link
Contributor

@MrAlias MrAlias commented Oct 3, 2022

instProvider type

Currently, instrument providers are defined for the async and sync instruments both of int64 and float64 types. That produces four instrument providers in total. Each instrument provider has three methods to create three different instruments. Each of these methods does the following:

  1. Create an instrument configuration
  2. Create a view.Instrument with the appropriate view.InstrumentKind based on the instrument provider being used.
  3. Resolve aggregators
  4. Validate the return of that resolution
  5. Return the instrument and error

For example,

// Counter creates an instrument for recording increasing values.
func (p asyncInt64Provider) Counter(name string, opts ...instrument.Option) (asyncint64.Counter, error) {
cfg := instrument.NewConfig(opts...)
aggs, err := p.resolve.Aggregators(view.Instrument{
Scope: p.scope,
Name: name,
Description: cfg.Description(),
Kind: view.AsyncCounter,
}, cfg.Unit())
if len(aggs) == 0 && err != nil {
err = fmt.Errorf("instrument does not match any view: %w", err)
}
return &instrumentImpl[int64]{
aggregators: aggs,
}, err
}

The only functionally unique part of these methods is in step 2 (step 5 technically involves a cast to an API type, but it ends up not being relevant).

These common functional elements can be abstracted into a common method. However, this can be taken further. Given each instrument provider would do this, we can define a common underlying type for all instrument providers that defines this method. E.g.

type instProvider[N int64 | float64] struct {
	resolve resolver[N]
}

func (p instProvider[N]) lookup(kind view.InstrumentKind, name string, opts []instrument.Option) (*instrumentImpl[N], error) {
	cfg := instrument.NewConfig(opts...)
	key := instProviderKey{
		Name:        name,
		Description: cfg.Description(),
		Unit:        cfg.Unit(),
		Kind:        kind,
	}

	aggs, err := p.resolve.Aggregators(key)
	if len(aggs) == 0 && err != nil {
		err = fmt.Errorf("instrument does not match any view: %w", err)
	}
	return &instrumentImpl[N]{aggregators: aggs}, err
}

Doing so, allows each instrument provider to be reduced to something resembling this:

type asyncInt64Provider struct {
	instProvider[int64]
}

func (p asyncInt64Provider) Counter(name string, opts ...instrument.Option) (asyncint64.Counter, error) {
	return p.lookup(view.AsyncCounter, name, opts)
}
func (p asyncInt64Provider) UpDownCounter(name string, opts ...instrument.Option) (asyncint64.UpDownCounter, error) {
	return p.lookup(view.AsyncUpDownCounter, name, opts)
}
func (p asyncInt64Provider) Gauge(name string, opts ...instrument.Option) (asyncint64.Gauge, error) {
	return p.lookup(view.AsyncGauge, name, opts)
}

This both reduces a lot of redundant code and centralizes the functional section development needs to deal with going forward.

Store instrument providers in a meter

Currently, when a meter is asked for a new instrument provider it creates a new one every time:

// AsyncInt64 returns the asynchronous integer instrument provider.
func (m *meter) AsyncInt64() asyncint64.InstrumentProvider {
return asyncInt64Provider{scope: m.Scope, resolve: newResolver[int64](m.pipes)}
}
// AsyncFloat64 returns the asynchronous floating-point instrument provider.
func (m *meter) AsyncFloat64() asyncfloat64.InstrumentProvider {
return asyncFloat64Provider{scope: m.Scope, resolve: newResolver[float64](m.pipes)}
}
// RegisterCallback registers the function f to be called when any of the
// insts Collect method is called.
func (m *meter) RegisterCallback(insts []instrument.Asynchronous, f func(context.Context)) error {
m.pipes.registerCallback(f)
return nil
}
// SyncInt64 returns the synchronous integer instrument provider.
func (m *meter) SyncInt64() syncint64.InstrumentProvider {
return syncInt64Provider{scope: m.Scope, resolve: newResolver[int64](m.pipes)}
}
// SyncFloat64 returns the synchronous floating-point instrument provider.
func (m *meter) SyncFloat64() syncfloat64.InstrumentProvider {
return syncFloat64Provider{scope: m.Scope, resolve: newResolver[float64](m.pipes)}
}

This includes allocating a pointer for a new resolver. An allocation is unavoidable given the resolver contains a slice and even if passed as a value, the new reference to a slice would incur an allocation.

As an alternative to this approach, allocate once-per-provider and do so at the start.

Performance considerations

Using the included benchmark for the change we can see this indeed reduces the overall allocations for new instrument creation:

name                      old time/op    new time/op    delta
InstrumentCreation-8        23.8µs ± 3%    21.4µs ± 4%  -10.13%  (p=0.000 n=17+17)

name                      old alloc/op   new alloc/op   delta
InstrumentCreation-8        11.4kB ± 0%    10.1kB ± 0%  -10.97%  (p=0.000 n=20+19)

name                      old allocs/op  new allocs/op  delta
InstrumentCreation-8           270 ± 0%       222 ± 0%  -17.78%  (p=0.000 n=20+20)

instProviderKey type

The resolver and inserter both accept a view.Instrument and unit.Unit. Unify these types into a single new instProviderKey that represents the new instrument request received by an instrument provider.

This enables easy caching for a future PR (see #3245)

Store the instrumentation.Scope in the insterter type instead of the meter

The meter type currently has a field that holds the associated instrumentation.Scope of the meter.

type meter struct {
instrumentation.Scope

This field is only maintained to pass to an instrument provider as one of its fields, which in turn passes it to a resolver for each instrument creation, which then passes it to an inserter. Instead of maintaining state in all these types, just have the inserter define it as a field directly.

@MrAlias MrAlias added pkg:SDK Related to an SDK package area:metrics Part of OpenTelemetry Metrics labels Oct 3, 2022
@MrAlias MrAlias added this to the Metric SDK: Beta milestone Oct 3, 2022
@MrAlias MrAlias added the Skip Changelog PRs that do not require a CHANGELOG.md entry label Oct 3, 2022
@codecov
Copy link

codecov bot commented Oct 3, 2022

Codecov Report

Merging #3256 (724893e) into main (496c086) will decrease coverage by 0.0%.
The diff coverage is 100.0%.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##            main   #3256     +/-   ##
=======================================
- Coverage   77.7%   77.6%   -0.1%     
=======================================
  Files        164     164             
  Lines      11651   11520    -131     
=======================================
- Hits        9054    8950    -104     
+ Misses      2388    2372     -16     
+ Partials     209     198     -11     
Impacted Files Coverage Δ
sdk/metric/instrument_provider.go 93.6% <100.0%> (+13.6%) ⬆️
sdk/metric/meter.go 100.0% <100.0%> (ø)
sdk/metric/pipeline.go 93.5% <100.0%> (+<0.1%) ⬆️
sdk/trace/batch_span_processor.go 80.2% <0.0%> (-1.8%) ⬇️
exporters/jaeger/jaeger.go 90.3% <0.0%> (-0.9%) ⬇️

@MrAlias MrAlias changed the title Restructure instrument providers Restructure instrument creation code paths Oct 3, 2022
@MrAlias MrAlias mentioned this pull request Oct 17, 2022
5 tasks
@MadVikingGod MadVikingGod linked an issue Oct 20, 2022 that may be closed by this pull request
5 tasks
@MrAlias MrAlias removed a link to an issue Oct 20, 2022
5 tasks
@MrAlias
Copy link
Contributor Author

MrAlias commented Oct 20, 2022

This doesn't close #3245, but it does block progress in adding caching to the instrument providers.

@MrAlias MrAlias removed this from the Metric SDK: Beta milestone Oct 20, 2022
@MrAlias MrAlias added this to the Metric v0.34.0 milestone Oct 27, 2022
sdk/metric/pipeline.go Outdated Show resolved Hide resolved
@MrAlias MrAlias merged commit acd8777 into open-telemetry:main Nov 11, 2022
@MrAlias MrAlias deleted the inst-prov-unify branch November 11, 2022 00:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:metrics Part of OpenTelemetry Metrics pkg:SDK Related to an SDK package Skip Changelog PRs that do not require a CHANGELOG.md entry
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

4 participants