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

Unify metric SDK caching #3245

Closed
3 of 5 tasks
MrAlias opened this issue Sep 29, 2022 · 4 comments
Closed
3 of 5 tasks

Unify metric SDK caching #3245

MrAlias opened this issue Sep 29, 2022 · 4 comments
Assignees
Labels
area:metrics Part of OpenTelemetry Metrics bug Something isn't working enhancement New feature or request pkg:SDK Related to an SDK package

Comments

@MrAlias
Copy link
Contributor

MrAlias commented Sep 29, 2022

Currently, the only place caching is used is the meterRegistry.

There are currently two reported bugs (#3229 and #3240) that would be resolved by adding caching to instrument aggregation determination. If done correctly the same request for compatible instruments would return the same Aggregator instance and conflicting, but resolvable, instruments would also get the same instance.

Finally, there is an optimization opportunity at the instrument provider level to cache returned instruments. Adding a cache at this level would prevent duplicate resolutions of aggregations.

Propoasal

A proof-of-concept with all three caching situations can be found here: https://github.com/MrAlias/opentelemetry-go/pull/662/files

The proposed approach is to add a unified cache type that handles synchronized lookup in a map:

type cache[K comparable, V any] struct {
	sync.Mutex
	data map[K]V
}

func (c *cache[K, V]) Lookup(key K, f func() V) V {
	c.Lock()
	defer c.Unlock()

	if c.data == nil {
		val := f()
		c.data = map[K]V{key: val}
		return val
	}
	if v, ok := c.data[key]; ok {
		return v
	}
	val := f()
	c.data[key] = val
	return val
}

This type can be wrapped to provide convenient lookup of aggregator caching: https://github.com/MrAlias/opentelemetry-go/blob/0d925dcfc651811318155a16bcc59c615ff119cf/sdk/metric/cache.go#L71-L147

Or it can be used to directly cache meter or instrument values.

The plan is to break this work into 3 PRs:

@MrAlias MrAlias added bug Something isn't working enhancement New feature or request pkg:SDK Related to an SDK package area:metrics Part of OpenTelemetry Metrics labels Sep 29, 2022
@MrAlias MrAlias added this to the Metric SDK: Beta milestone Sep 29, 2022
@MrAlias MrAlias self-assigned this Sep 29, 2022
@MadVikingGod MadVikingGod linked a pull request Oct 20, 2022 that will close this issue
@MrAlias MrAlias removed this from the Metric SDK: Beta milestone Oct 20, 2022
@MadVikingGod
Copy link
Contributor

@MrAlias Can you elaborate on the final bullet point?

@MrAlias
Copy link
Contributor Author

MrAlias commented Dec 6, 2022

@MrAlias Can you elaborate on the final bullet point?

Create a cache and use it for this method:

func (p *instProvider[N]) lookup(kind InstrumentKind, name string, opts []instrument.Option) (*instrumentImpl[N], error) {

@MrAlias
Copy link
Contributor Author

MrAlias commented Dec 8, 2022

We should split off this level of caching for its own issue and assign to the post-GA phase.

@MrAlias
Copy link
Contributor Author

MrAlias commented Dec 8, 2022

Remaining task split into #3527

@MrAlias MrAlias closed this as completed Dec 8, 2022
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 bug Something isn't working enhancement New feature or request pkg:SDK Related to an SDK package
Projects
No open projects
Status: Done
Development

No branches or pull requests

2 participants