opentelemetry-collector icon indicating copy to clipboard operation
opentelemetry-collector copied to clipboard

[Proposal]: Enhance [*]Flags to not need [*]Flag, and to simplify usage

Open bogdandrutu opened this issue 3 years ago • 1 comments

Current API(s):

Right now users can check for a specific flag by using HasFlag API, which hides some complicated logic of using bit operations, but requires multiple structs/types.

// MetricDataPointFlags defines how a metric aggregator reports aggregated values.
// It describes how those values relate to the time interval over which they are aggregated.
type MetricDataPointFlags uint32

const (
	// MetricDataPointFlagsNone is the default MetricDataPointFlags.
	MetricDataPointFlagsNone = MetricDataPointFlags(otlpmetrics.DataPointFlags_FLAG_NONE)
)

// NewMetricDataPointFlags returns a new MetricDataPointFlags combining the flags passed
// in as parameters.
func NewMetricDataPointFlags(flags ...MetricDataPointFlag) MetricDataPointFlags {
	var flag MetricDataPointFlags
	for _, f := range flags {
		flag |= MetricDataPointFlags(f)
	}
	return flag
}

// HasFlag returns true if the MetricDataPointFlags contains the specified flag
func (d MetricDataPointFlags) HasFlag(flag MetricDataPointFlag) bool {
	return d&MetricDataPointFlags(flag) != 0
}

// String returns the string representation of the MetricDataPointFlags.
func (d MetricDataPointFlags) String() string {
	return otlpmetrics.DataPointFlags(d).String()
}

// MetricDataPointFlag allow users to configure DataPointFlags. This is achieved via NewMetricDataPointFlags.
// The separation between MetricDataPointFlags and MetricDataPointFlag exists to prevent users accidentally
// comparing the value of individual flags with MetricDataPointFlags. Instead, users must use the HasFlag method.
type MetricDataPointFlag uint32

const (
	// MetricDataPointFlagNoRecordedValue is flag for a metric aggregator which reports changes since last report time.
	MetricDataPointFlagNoRecordedValue = MetricDataPointFlag(otlpmetrics.DataPointFlags_FLAG_NO_RECORDED_VALUE)
)

Proposal API:

Essentially hide the bit operation from the users and provide helpers to access different flags from the bitset

// MetricDataPointFlags defines how a metric aggregator reports aggregated values.
// It describes how those values relate to the time interval over which they are aggregated.
type MetricDataPointFlags uint32

// NewMetricDataPointFlags returns a new MetricDataPointFlags combining the flags passed
// in as parameters.
func NewMetricDataPointFlags() MetricDataPointFlags {
	return MetricDataPointFlags(otlpmetrics.DataPointFlags_FLAG_NONE)
}

// NoRecordedValue returns true if the MetricDataPointFlags contains the specified flag
func (d MetricDataPointFlags) NoRecordedValue() bool {
	return d&MetricDataPointFlags(otlpmetrics.DataPointFlags_FLAG_NO_RECORDED_VALUE) != 0
}

// HasFlag returns true if the MetricDataPointFlags contains the specified flag
func (d MetricDataPointFlags) SetNoRecordedValue(val bool) {
	....
}


// String returns the string representation of the MetricDataPointFlags.
func (d MetricDataPointFlags) String() string {
	return otlpmetrics.DataPointFlags(d).String()
}

bogdandrutu avatar May 31 '22 18:05 bogdandrutu

@bogdandrutu would SetNoRecordedValue(false) remove the flag?

TylerHelmuth avatar Jun 20 '22 16:06 TylerHelmuth

@TylerHelmuth Should we do the same for TraceFlags in the LogRecord? See https://github.com/open-telemetry/opentelemetry-collector/blob/d5e80be6f61eb9e354691466f37d7250671513e3/pdata/internal/generated_plog.go#L631?

bogdandrutu avatar Aug 08 '22 16:08 bogdandrutu

@bogdandrutu ya best to keep the experience consistent within pdata

TylerHelmuth avatar Aug 15 '22 15:08 TylerHelmuth

All places updated, we need only to finish the deprecation.

bogdandrutu avatar Aug 17 '22 16:08 bogdandrutu