New immutable type for Flags structs

[bogdandrutu]

This is not a breaking change, since the LogRecordFlags and the getter/setter to LogRecords were never released.

Even though the Contrib tests are failing, this is expected because contrib was updated to an unrelease version to simplify the next release.

Signed-off-by: Bogdan [email protected]

[tigrannajaryan]

I think this nicely encapsulates the types and follows the separation of concerns between OTLP and W3C, which is great.

However, I am not sure the ergonomics of using the resulting API are very good. So for instance to set the sampled flag on an existing LogRecord you need to do:

logRecord.SetFlagsStruct(logRecord.FlagsStruct().WithTraceFlags(logRecord.FlagsStruct().TraceFlags().WithIsSampled(true)))

This seems a bit too much to flip a single bit.

What do other @open-telemetry/collector-approvers think?

(I know I asked for this, but I am happy to admit my proposal was wrong if that's how people feel about the API).

[bogdandrutu]

@tigrannajaryan this is a draft, so indeed some comments are incorrect, but will fix them

Regarding the example, indeed is too much if you do it in 1 line with no local vars.

No local vars:

logRecord.SetFlagsStruct(logRecord.FlagsStruct().WithTraceFlags(logRecord.FlagsStruct().TraceFlags().WithIsSampled(true)))

TraceFlags local var:

tf := logRecord.FlagsStruct().TraceFlags()
logRecord.SetFlagsStruct(logRecord.FlagsStruct().WithTraceFlags(tf.WithIsSampled(true))

LogRecordFlags local var:

lf := logRecord.FlagsStruct()
logRecord.SetFlagsStruct(lf.WithTraceFlags(lf.TraceFlags().WithIsSampled(true))

LogRecordFlags and TraceFlags local vars:

lf := logRecord.FlagsStruct()
tf := tf.TraceFlags()
logRecord.SetFlagsStruct(lf.WithTraceFlags(tf.WithIsSampled(true))

/cc @TylerHelmuth please comment since you also worked on this.

Update: The FlagsStruct is temporary, final name will be Flags.

[codecov]

Codecov Report

Base: 92.17% // Head: 92.16% // Decreases project coverage by -0.00% ⚠️

Coverage data is based on head (f59888f) compared to base (9ba35cd).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5972      +/-   ##
==========================================
- Coverage   92.17%   92.16%   -0.01%     
==========================================
  Files         210      212       +2     
  Lines       13272    13259      -13     
==========================================
- Hits        12233    12220      -13     
  Misses        822      822              
  Partials      217      217              

Impacted Files	Coverage Δ
pdata/internal/wrapper_logs.go	100.00% <ø> (ø)
pdata/plog/logs.go	83.87% <ø> (-6.52%)	⬇️
exporter/loggingexporter/internal/otlptext/logs.go	100.00% <100.00%> (ø)
pdata/internal/generated_wrapper_logs.go	100.00% <100.00%> (ø)
pdata/pcommon/trace_flags.go	100.00% <100.00%> (ø)
pdata/plog/generated_logs.go	96.79% <100.00%> (+0.02%)	⬆️
pdata/plog/log_record_flags.go	100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

[TylerHelmuth]

@bogdandrutu @tigrannajaryan reading through this it definitely feels like a deviation away from the simplicity of LogRecordFlags.SetIsSampled or the original solution of Set with constants. If the increased complexity is providing value I'm all for it but I think I'm missing it.

Even with local vars, logRecord.SetFlagsStruct(lf.WithTraceFlags(tf.WithIsSampled(true)) seems verbose. My first question when reading it was "why am I setting the flags and then passing the flags to a setter?". I think the confusion is centered around inclusion of logRecord.FlagsStruct().WithTraceFlags(). To me, that API reads like I am about to update the FlagsStruct to include new/modified TraceFlags, so it was jarring to see the result being passed into SetFlagsStruct.

I am not as knowledgeable on OTLP/WC3/pdata as you two, so I am not immediately seeing the need for this change. My initial opinion is I don't love the complexity/ergonomics this adds.

[tigrannajaryan]

@TylerHelmuth for the context, I think this was triggered by my comment here #5959 (comment)
While I do feel separation of concerns between OTLP and W3C is important, I am not sure it is important enough to warrant a somewhat less ergonomic API. Hard to tell what's best, it is subjective and I am happy with either approach the majority of approvers/maintainers support.

[bogdandrutu]

TL;DR: Based on the current investigation I am proposing to go with an immutable API for all flags implementations to avoid AsRaw/FromRaw APIs, and if possible to also go with the separation of concerns that @tigrannajaryan mentioned.

There are other examples that need to be considered as well (not only the most unfavorable case), will try to compare both APIs, while doing so will use same name Flags for the funcs to access. There are actually 2 different decisions to be made here:

1. the separation of concerns - this is what @tigrannajaryan was mentioning
2. but also between immutable vs mutable implementation for flags.

Immutable vs Mutable

Also a questions from previous comments "why am I setting the flags and then passing the flags to a setter?"

There is no downside on retrieving/checking the flag components.

So we need to make a tradeoff between having these AsRaw/FromRaw on a mutable Flags implementation which does not require the "Set" after change, or have immutable Flags which require set after changes, but conversion to/from raw value is implicit type XYZFlags uint32.

The immutable pattern in golang is not new, since types like time.Time or time.Duration will have to do the same, because they are immutable so cannot mutate them in place.

isSampled := logRecord.Flags().IsSampled()
// OR
isSampled := logRecord.Flags().IsSampled()

Updating a value:

isSampled := logRecord.Flags().SetIsSampled(true)
// OR
isSampled := logRecord.SetFlags(logRecord.Flags().WithIsSampled(true))

Accessing raw:

var rawFlags uint32

rawFlags = logRecord.Flags().AsRaw()
// OR
rawFlags = logRecord.Flags()

Updating from raw:

logRecord.Flags().FromRaw(rawFlags)
// OR
logRecord.SetFlags(rawFlags)

After crafting this PR I kind of like this, the downside of setting the value is less important in practice than the conversion to/from raw (don't show me the contrib usages of SetIsSampled, because I know all of them, since I added all of them, they are there just because before in tests users set a random value for the flags which was not possible with the current API without FromFlags which PR generated this discussion). This does not imply that we need a TraceFlags redirection, we can have the IsSampled and WithIsSampled directly on the LogRecordFlags, the decision about that is part of the "separation of concerns" problem.

Separation of Concerns between W3C and OTLP

In other words, the OTLP defines the least significant byte as the w3c trace flags, so the meaning of the values inside that byte are delegated to the w3c definition. The separation of concerns is a nice to have, but we need to understand that it comes with a "redirect" cost. So code will have to write Flags().TraceContext().IsSampled() to retrieve the sampling bit, which indeed is defined by the w3c standard.

This has also a benefit (not only downsides), because this avoids duplicate code, in an imminent event of adding TraceFlags in other places, we will need to duplicate these funcs (IsSampled, etc.) if we don't do the indirect.

Retrieving a value:

isSampled := logRecord.Flags().IsSampled()
// OR
isSampled := logRecord.Flags().TraceFlags().IsSampled()

Updating a value:

isSampled := logRecord.Flags().SetIsSampled(true)
// OR
isSampled := logRecord.Flags().TraceFlags().SetIsSampled(true)

Accessing raw trace context only part:

logRecord.Flags().AsRaw()&0xFF
// OR
logRecord.Flags().TraceContext().AsRaw(rawFlags)

Updating from raw trace context only part:

logRecord.Flags().FromRaw((logRecord.Flags().AsRaw & ^0xFF) | rawFlags)
// OR
logRecord.Flags().TraceContext().FromRaw(rawFlags)

After crafting this PR I kind of like this, since will avoid any duplicate code.

Bogdan's current proposal

This is the current PR, which combines the immutable and separation of concerns (just for your information the mutable and separation of concerns is impossible to build with the current pdata framework without an extra allocation OR usage of unsafe, because TraceFlags will require a pointer to uint8 value, but LogRecordFlags has a pointer to a uint32, which means you need to create a pointer to the least significant byte there, which is possible only with unsafe).

Retrieving a value:

isSampled := logRecord.Flags().TraceFlags().IsSampled()

Updating a value inside TraceFlags part:

lf := logRecord.Flags()
logRecord.SetFlags(lf.WithTraceFlags(lf.TraceFlags().WithIsSampled(true))

Updating a value outside TraceFlags part:

logRecord.SetFlags(logRecord.Flags().WithFoo(true))

Accessing raw:

rawFlags = logRecord.Flags()

Updating from raw:

logRecord.SetFlags(rawFlags)

Accessing raw trace context only part:

rawTraceFlags = logRecord.Flags().TraceFlags()

Updating from raw trace context only part:

logRecord.SetFlags(logRecord.Flags().WithTraceFlags(rawTraceFlags)

[tigrannajaryan]

This has also a benefit (not only downsides), because this avoids duplicate code, in an imminent event of adding TraceFlags in other places

I assume this is an example of possible such addition, right?

Bogdan's current proposal

This proposal aligns with what I was suggesting. I think it is the strongest from the perspective of safety and separation of concerns. I am only reluctant because the ergonomics are not very good.

Accessing raw:

rawFlags = logRecord.Flags()

Updating from raw:

logRecord.SetFlags(rawFlags)

Accessing raw trace context only part:

rawTraceFlags = logRecord.Flags().TraceFlags()

Updating from raw trace context only part:

logRecord.SetFlags(logRecord.Flags().WithTraceFlags(rawTraceFlags)

Where do we need these raw functions?

[bogdandrutu]

I assume open-telemetry/opentelemetry-proto#384 of possible such addition, right?

Yes

This proposal aligns with what I was suggesting. I think it is the strongest from the perspective of safety and separation of concerns. I am only reluctant because the ergonomics are not very good.

I believed so, since was inspired by your comments.

Where do we need these raw functions?

Next section has a summary of current usages of the funcs:

Where do we need these getters?

• For conversions to/from data formats where the concepts have an equivalent in that format. For example in the metrics flags, they have the no recorded value property (equivalent with stale value), in the prometheus receiver/exporter.
• In unit-tests to generate input values for previous cases and to generate values for all the fields in a log/metric.

Where do we need these setters?

• For conversions to/from data formats where the concepts have an equivalent in that format. For example in the metrics flags, they have the no recorded value property (equivalent with stale value), in the prometheus receiver/exporter.
• In unit-tests to generate input values for previous cases and to generate values for all the fields in a log/metric.

Where do we need these raw functions?

• Conversion to/from different data formats where this information is encoded differently (as hex, or base64). For example in the stanza conversion.
• In tests for these conversion algorithms to set a value that is not possible to set via setters.
• For the moment in the transform processor, not sure that this is ok, but right now we allow users to get and set the raw values. (Don't want this to become the subject, so take it as a current use-case that may be changed to setter and getter).

In general, I don't see a use-case for setters except conversions between formats, of something like the "transform" processor. In the type conversions we have lots of "exporters" so the getters are more important there.

[TylerHelmuth]

@bogdandrutu thank you for the in-depth analysis.

Understanding the situation better, I can see why we'd do something like

isSampled := logRecord.SetFlags(logRecord.Flags().WithIsSampled(true))

to achieve immutability. I think my dislike is stemming from the ergonomics introduced by TraceFlags. I understand now why the introduction could be helpful, but I still don't love doing this

lf := logRecord.Flags()
logRecord.SetFlags(lf.WithTraceFlags(lf.TraceFlags().WithIsSampled(true))

Personally, I'd rather duplicate code than introduce that API, especially if the duplicated code could get autogenerated by pdata.

Also, I anticipate there being questions raised as to why the API for updating the flags is different for flags inside the trace part vs outside the trace part. Not saying that is isn't correct, only that I think it will raise questions our community will have to answer.

[tigrannajaryan]

I think my dislike is stemming from the ergonomics introduced by TraceFlags. I understand now why the introduction could be helpful, but I still don't love doing this

Yes. I think there is value in separation of concerns, but it is clear that it results in less ergonomic API. I don't think it is possible to make this choice objectively, so let's make a subjective call ("I like this and not that"). I am fine with whatever we choose approach, no strong preference.

Personally, I'd rather duplicate code than introduce that API, especially if the duplicated code could get autogenerated by pdata.

I support this. It is a small amount of duplication. The TraceFlags are unlikely to become something complicated. It is a small surface area and it is not a big deal to duplicate the API where it is needed.

[bogdandrutu]

Closing in favor of:

• Change MetricDataPointFlags to the new immutable pattern #5999
• New immutable type for LogRecordFlags structs #5996