oxidecomputer / oxidecomputer/omicron

`oximeter`'s byte array datum could use some thought

Open
#4,551 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

oximeter
Dominant language
Rust
Stars
572
Forks
97
Avg merge
2d 12h
Merged PRs (30d)
96

Description

To implement #4311, supporting missing samples, it makes sense to store each datum type in a nullable column in ClickHouse. For example, Cumulative<u32>s would be represented in a column with type Nullable(UInt32). However, the nullable wrapper type cannot contain composite types, which includes arrays, which we use to represent both histograms and arbitrary byte arrays. The former we can handle unambiguously: it's not possible to create an oximeter::Histogram<T> with zero bins, so we can use an empty array as a sentinel for a missing sample. This doesn't work for byte arrays, or it at least raises a number of questions:

  1. If we can't store bytes in the database as Nullable(Array(UInt8)), what type can we use? ClickHouse's String type is a C++-style string, which means it's really just an array of octets. We could use this, but it interacts poorly with the JSON encoding we're currently using to talk to the database. I don't believe it's possible to round-trip an escaped "byte string" to the database via JSON. We could do this if we switch to a binary serialization format, such as RowBinary.
  2. Even if we could do some work to encode the bytes as a string on the way in and out of the database, this makes it difficult to use correctly. First, if you access the database out of band of this client, you need to know the encoding and how to reverse it. Second, the fact that you need to encode or decode the value is not present in the database's type information -- there is no separate BLOB type, since that's just an alias for String, and so one would need to know which table the value was extracted from to know whether decoding was required. That becomes even trickier if / when we allow selecting data from more than one table. E.g., what happens if someone selects a "true" string and a byte array from two tables? It's not impossible to handle, it just can't happen directly at the serialization layer.
  3. We could always choose to encode the Datum::Bytes object itself. I.e., instead of storing a byte array inside that, we could just decide to always base-64 encode the data, and store things as a string. This has the benefit of being more obvious, but still presents some of the problems in (2).

There is another option: we remove support for byte arrays. No code produces byte array samples today. The type was intended as an escape hatch of sorts, or a way to collect arbitrary information. But one can still always do that through the string type, where the application decides how the arbitrary data is encoded. For example, nothing prevents someone from storing JSON or base-64 encoded bytes in the Datum::String today.

I'm partial to this last choice, but there could also be a way to correctly and unambiguously store bytes that I've not considered.

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start by reviewing #4311 and the oximeter Datum::Bytes storage path, then evaluate the ClickHouse Nullable, array, String, JSON, and RowBinary constraints described here. Done requires an agreed, unambiguous treatment of byte-array samples and its implementation or removal, but this issue names no file or test to run.

Written by the indexing model from the issue text.

Assessment

Tech stack
rust
Domain
backend-api-design, databases
Issue type
Feature
Difficulty
5/5
Estimated time
Over a week
Activity status
Stale
Clarity
Needs clarification
Newbie friendliness
25/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.