Skip to content

Merge Int64DataPoint and DoubleDataPoint into ScalarDataPoint#172

Closed
jmacd wants to merge 2 commits intoopen-telemetry:masterfrom
jmacd:jmacd/scalar_val
Closed

Merge Int64DataPoint and DoubleDataPoint into ScalarDataPoint#172
jmacd wants to merge 2 commits intoopen-telemetry:masterfrom
jmacd:jmacd/scalar_val

Conversation

@jmacd
Copy link
Copy Markdown
Contributor

@jmacd jmacd commented Jul 20, 2020

The decision to unify the two SCALAR value types into a single data point message type is justified as follows:

  1. This reduces the size of Metric by one repeated field (in Go 24 bytes)
  2. This grows the corresponding data points slightly (in Go 8 bytes)
  3. The marginal memory cost of carrying both a int64 and double value in the same structure is minor, because that type already contains three other fields, one of them repeated. E.g., we have two 64 bit timestamps, one slice (3 x 64 bits), plus with the Google protoc-gen-go changes in v1.23.0 we have an additional pointer, uint32, and slice known as MessageState, SizeCache, and UnknownFields. Taken together, the Go ScalarDataPoint is less than 10% larger than the former Int64DataPoint.
  4. This can result in simpler implementation..
  5. Note that Add Exemplar support to Metrics proto #159 will replace the lost repeated field in this PR with a new repeated field of raw values, so this combined with Add Exemplar support to Metrics proto #159 will give a nearly break-even change.

@jmacd jmacd requested review from a team July 20, 2020 08:08
@jmacd
Copy link
Copy Markdown
Contributor Author

jmacd commented Jul 20, 2020

From #168 (comment)

The decision to unify the two SCALAR value types into a single data point message type is justified as follows: the marginal memory cost of carrying both a int64 and double value in the same structure is minor, because that type already contains three other fields, one of them repeated. E.g., we have two 64 bit timestamps, one slice (3 x 64 bits), plus with the Google protoc-gen-go v1.23.0 we have an additional pointer, uint32, and slice known as MessageState, SizeCache, and UnknownFields. Taken together, the Go ScalarDataPoint is less than 10% larger than the former Int64DataPoint.

This is a justification that it does not impact that much memory but not a justification why it benefits

The benefits are stated as (1), (4), and (5) in the description. It's (5) that I'm after.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants