Add FromRaw to Flags struct

[bogdandrutu]

See usages in open-telemetry/opentelemetry-collector-contrib#13617 where we had to check for the sample bit to set the value instead of just setting the entire value.

Signed-off-by: Bogdan [email protected]

[codecov]

Codecov Report

Merging #5959 (0617770) into main (58fa059) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main    #5959   +/-   ##
=======================================
  Coverage   92.30%   92.30%           
=======================================
  Files         212      212           
  Lines       13249    13253    +4     
=======================================
+ Hits        12229    12233    +4     
  Misses        803      803           
  Partials      217      217           

Impacted Files	Coverage Δ
pdata/plog/logs.go	90.74% <100.00%> (+0.35%)	⬆️
pdata/pmetric/metrics.go	92.80% <100.00%> (+0.11%)	⬆️

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

[tigrannajaryan]

Instead of having both IsSampled and FromRaw I think a better way would be to do this:

1. Declare new type LogRecordFlags.
2. Add IsSampled/SetIsSampled methods to LogRecordFlags.
3. Have LogRecord.SetFlags/GetFlags which uses LogRecordFlags.
4. Delete IsSampled/SetIsSampled/FromRaw methods from LogRecordFlags

[bogdandrutu]

So to set the flag you need:

flags := logRecord.Flags()
flags.SetIsSampled(true)
logRecord.SetFlags(flags)

This is a bit inconsistent with current pdata similar structs. It is similar with TraceID/SpanID in that regard, but these are immutable types. The rule is that if an object is "mutable" (SetIsSampled) then we don't offer a setter to avoid mistakes when updated but not "re-set".

[tigrannajaryan]

The reason I believe what I suggest is the right approach is separation of concerns. OTLP is not concerned with "sampled" flag directly. OTLP is concerned that there is a byte-sized W3C flags. That's all OTLP needs to know. The inner structure of W3C byte flag is W3C concern. pdata is our abstracted representation of OTLP and must be concerned with the same concepts as OTLP, not more, not less. In the spirit of this I believe pdata API must expose W3C flags as a byte (aliased as a new type for stronger typing), but not individual bits in the W3C flags.

So, perhaps a slightly different variation of what I suggested which is more consistent with my reasoning above would be to have this:

1. Declare new type W3CTraceFlags (an alias of byte).
2. Add IsSampled/SetIsSampled methods to W3CTraceFlags.
3. Have LogRecord.SetTraceFlags/GetTraceFlags which uses W3CTraceFlags.
4. Delete IsSampled/SetIsSampled/FromRaw methods from LogRecordFlags.

[bogdandrutu]

pdata is our abstracted representation of OTLP and must be concerned with the same concepts as OTLP

This is where I disagree, pdata is our collector data model, which is based on "OTLP" but is not "OTLP". It is the collector's data model. The structs in ptraceotlp are related to OTLP (protocol), the others are collector's data model, indeed based on OTLP.

So, perhaps a slightly different variation of what I suggested which is more consistent with my reasoning above would be to have this:

Even this has the downside I mentioned, this does not address that, on how to change the "Sampled" flag.

[bogdandrutu]

An option is to make the Flags immutable and have a "WithIsSampled" which returns a new Flags struct, so then we are consistent that we have setters only for immutable types.

[tigrannajaryan]

This is where I disagree, pdata is our collector data model, which is based on "OTLP" but is not "OTLP". It is the collector's data model.

Indeed. If that's the way we see it then we are free to deviate from OTLP's understanding. However, do we? What do we gain by that deviation?

On the contrary, there is value in saying "pdata is an efficient in-memory representation and API to work with OTLP data". I envision it being a separate Otel library, not even necessarily part of Collector. Any Go codebase that needs to work with OTLP data can use such a library. Think for example Schema transformation. This is now developed as a new processor in Collector, however backends also need similar functionality. If pdata becomes an independent Otel library, then Schema transformer can also be an independent Otel library that depends on pdata, but is not in the Collector codebase. Then in the Collector codebase we can have just the schematransform processor which uses both libraries.

[bogdandrutu]

That is fine, still I believe that we should respect the rule that we "Set" only immutable types, so we cannot have SetIsSampled, we need to return a new type to keep the logic that all types that we offer setters are immutable.

[tigrannajaryan]

I don't think it matters for non-reference types (like the proposed W3CTraceFlags). The rule that types with setters need to be immutable is only important for reference types so that you can't use the mutability to modify the value without explicitly calling the setter. For non-reference types this is impossible, so not an issue.
Anyway, I am fine either way, this is a minor issue, works either way for me.

[bogdandrutu]

@tigrannajaryan please review #5972 (I closed #5970 since is way more complicated).

[bogdandrutu]

Closing in favor of:

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