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

Implement the sdk/metric/number package #2841

Closed
4 tasks
MadVikingGod opened this issue Apr 21, 2022 · 3 comments
Closed
4 tasks

Implement the sdk/metric/number package #2841

MadVikingGod opened this issue Apr 21, 2022 · 3 comments
Labels
area:metrics Part of OpenTelemetry Metrics pkg:SDK Related to an SDK package

Comments

@MadVikingGod
Copy link
Contributor

MadVikingGod commented Apr 21, 2022

Blocked by #2799

  • Implement the sdk/metric/number package
  • Ensure that the package is tested
  • Document the package in the godocs
  • Ensure all implementation TODOs have an issue tracking them.
@MadVikingGod MadVikingGod added pkg:SDK Related to an SDK package area:metrics Part of OpenTelemetry Metrics labels Apr 21, 2022
@MadVikingGod MadVikingGod changed the title Add sdk/metric/number structure to new_sdk/main Implement the sdk/metric/number package Apr 21, 2022
@MrAlias
Copy link
Contributor

MrAlias commented Apr 21, 2022

Should the number and sdkinstrument package be combined into a single instrument package that instead calls the Number type Value?

@MrAlias
Copy link
Contributor

MrAlias commented Jun 22, 2022

I think we can avoid exporting a number package entirely. I added the following to the the sdk/metric/internal package:

// Atomic provides atomic access to a generic value type.
type Atomic[N int64 | float64] interface {
	// Store value atomically.
	Store(value N)

	// Add value atomically.
	Add(value N)

	// Load returns the stored value.
	Load() N

	// Clone creates an independent copy of the current value.
	Clone() Atomic[N]
}

type Int64 struct {
	value *int64
}

var _ Atomic[int64] = Int64{}

func NewInt64(v int64) Int64 {
	return Int64{value: &v}
}

func (v Int64) Store(value int64) { atomic.StoreInt64(v.value, value) }
func (v Int64) Add(value int64)   { atomic.AddInt64(v.value, value) }
func (v Int64) Load() int64       { return atomic.LoadInt64(v.value) }
func (v Int64) Clone() Atomic[int64] {
	return NewInt64(v.Load())
}

type Float64 struct {
	value *uint64
}

var _ Atomic[float64] = Float64{}

func NewFloat64(v float64) Float64 {
	u := math.Float64bits(v)
	return Float64{value: &u}
}

func (v Float64) Store(value float64) {
	atomic.StoreUint64(v.value, math.Float64bits(value))
}

func (v Float64) Add(value float64) {
	for {
		old := atomic.LoadUint64(v.value)
		sum := math.Float64bits(math.Float64frombits(old) + value)
		if atomic.CompareAndSwapUint64(v.value, old, sum) {
			return
		}
	}
}

func (v Float64) Load() float64 {
	return math.Float64frombits(atomic.LoadUint64(v.value))
}

func (v Float64) Clone() Atomic[float64] {
	return NewFloat64(v.Load())
}

This was then used by the aggregators to atomically aggregate values in the proof-of-concept I made from #2954.

I think avoiding exposing this implementation detail is advantageous. It will provide flexibility in the future, stops us from tracking a number type everywhere, and reduces the amount of exported API we will need to maintain in the future.

@MrAlias
Copy link
Contributor

MrAlias commented Jul 7, 2022

It seems like this package may be avoidable based on the current direction the SDK (specifically the aggregations) is going.

Closing to remove possibly stale work. We can reopen this if it is deemed necessary in the future.

@MrAlias MrAlias closed this as completed Jul 7, 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 pkg:SDK Related to an SDK package
Projects
No open projects
Development

No branches or pull requests

2 participants