Only support signed 64 bit integers as attribute values

[pyohannes]

For attribute values, the specification only asks for one integer value: signed 64 bit integers.

However, currently our API currently supports four integer types:

• uint32_t
• uint64_t
• int32_t
• int64_t

OTLP also only supports signed 64 bit integers, so we will run into problems with unsigned 64 bit integers there.

I recommend aligning with the specification and to only support int64_t. I can put up a PR for that, but this is breaking change, so please give your opinions here.

[maxgolov]

I think this is a disaster in the making.

[maxgolov]

Let me elaborate: certain metrics flows rely on unsigned 64-bit integers. Most mature tracing systems support the full gamut of types. It is always easy to violently convert any other type to int64_t for OTLP exporter, if that is the only type to be supported by OTLP exporter, but it is much harder to get back and reconstruct unsigned 64-bit integer from int64_t if typing information is lost right away on API surface.

[maxgolov]

Also narrowing it down to just int64_t - will present ABI compatibility challenge later on for C++ API, should any of these other types get added in future. I think from the language perspective and from the future-proofing perspective - you still have to accept Integral or Float type on API surface. No matter what number type it is on API surface [8|16|32|64]bit x [signed|unsigned] ... Then convert it to whatever is the type supported by concrete exporter...

And if concrete exporter cannot express unsigned 64-bit integers - well, I guess then someone can come up with an exporter customization that can convert a large number to string. And append the relevant type-hinting attribute fields, that would help to identify that, indeed, my Payload["myNumber"]="0xFFFFFFFFFFFFFF" is 64-bit unsigned, by also appending Payload["myNumber.Type"]="uint64_t" or alike... So then who needs those numbers, still get them and pays the price to send an extra width or size attribute.

From ETW exporter perspective that supports much wider typing system (and subsequently many other databases that may store strongly typed key-value pairs) - simple types here , I'd say that restricting this to just int64_t on API surface would be big a mistake. Structured events should be able to carry not just int64_t's ...

[lalitb]

Agree @maxgolov typing information could to relevant to many telemetry systems, and loosing this at api level won't be good. As you mentioned, we need to find ways to preserve type in such scenarios.

[pyohannes]

No matter what number type it is on API surface [8|16|32|64]bit x [signed|unsigned] ... Then convert it to whatever is the type supported by concrete exporter...

I see, so our policy will be to accept everything on the API side and let the exporters handle the details.

In light of that, I don't see why we shouldn't include nostd::span<const uint8_t> out of the box. It seems like a case similar to having uint64_t in the API: it's not in the spec, but we leave it to each exporter to decide how to handle it.

[ThomsonTan]

@pyohannes even with nostd::span<const uint8_t>, the type information is still lost. Could we request the spec to support all these types as they are important?

[maxgolov]

@pyohannes - makes sense to me! We can ensure that we add a snippet to our documentation, something like:

Please make sure that the value types passed to attributes are compatible with your exporter. Default OTLP exporter implementation allows for the following types (link) ... API surface enables type casting, as in changing an variable of one data type into another when and if it makes sense. For example, 32-bit signed integers passed to API may be transformed to 64-bit signed integers. However, unsigned 64-bit integers are not supported by OTLP exporter. Please avoid passing types that are not compatible with the exporter of your choice.

That should be sufficient. As long as the contract is clearly spelled out in the documentation, we are good with respect to remaining fully compliant with the mainline spec.

[maxgolov]

Related issue about adding byte buffers on API: open-telemetry/opentelemetry-specification#780

If we "future-proof" the API surface for v1.0 C++ SDK now, then it's going to be easier for us to support new types in v1.1 or v1.2 without needing to rev-up. ABI compatibility is a pain for C++ SDK, so it's better if we future-proof. Easier to tell that we have types "reserved for future use" than trying to add them later. Other languages are not generally prone to ABI issues. And in lesser need for future proofing. We need to do what is best for C++ as a language.

[lalitb]

Does it mean that we would keep supporting all the types for attribute values at api surface, and let exporter decide how to handle these data? Won't that be non-compliance with api specs ?

[maxgolov]

Won't that be non-compliance with api specs ?

We can provide the list of types that are supported by OTLP exporter. And explain that any 'non-standard' type behavior is reserved for future use , due to unique ABI compatibility requirements of native code (other languages generally do not have these strict requirements). There is always an option to violently cast uint64_t to int64_t, or convert a uint8_t to int64_t on wire. There is also an option to accept the byte buffer, but transform it into base64 string, should exporter dev be willing to do that conversion in the SDK.. Because otherwise, - how would you send a byte buffer if you do not have it in the typing system. Are we forcing native C / C++ devs to forget that byte buffers and uint64_t numbers exist?

@TomRoSystems @pyohannes @ThomsonTan

During the meeting yesterday, I promised to follow-up with the list of practical reasons to advocate for inclusion of:

• uint64_t type; and
• byte array type; and
• possibly other fundamental types

These are the reasons:

1. DTrace is a prominent trace framework for Sun, Mac and Microsoft maintains a port of it for Windows and Linux. DTrace supports the wide gamut of types, specifically the entirety of Integer primitive types (including uint64_t that we are discussing, as well as byte buffer arrays that I advocated for inclusion in the next rev of spec and saw no objections from the committee for including it after GA ). So one designing a shim / exporter from DTrace traces to OpenTelemetry should take that into consideration.

2. Google API Discovery Service contains a good list of types. And what happens to the types if they cannot be natively expressed in JSON notation, for example? There is extra type hinting / type format preserved. There is an assumption that APIs may still accept parameters as uint64_t, but that is transformed into type format / type hint on wire. If you are designing an APM solution that is expected to trace API calls, that APM solution should respect the wide gamut of types. For example, span->AddEvent("MyEventName", {{"call", "MyAPICallWithUint64Param"}, {"uint64key", 0xFFFFFFFFFFFFFFFF}}) - is a valid construct in scope of APM. Without that construct we are inherently unable to trace the API call parameters properly.

3. In embedded engineering, e.g. CPU performance counters on ARM architecture, are 64-bit wide, not negative. If one to design a solution that tracks the counter value in native code , it has to respect the fact that it would largely deal with unsigned 64-bit integers.

4. Android Tracing framework. Source code here relies on uint64_t tags. These do not directly map to concept of SpanId or TraceId. And the actual unsigned value of these tags should be respected in E2E APM system, esp if it needs to deal with Mobile Observability scenarios.

I would say we should future-proof it, by describing that the extra types are reserved for future use. That is not non-compliance, in my opinion. That is reasonable effort to keep C++ API stable, for when the spec finally adds the new types. Even in C++ standard you can have non-standard extensions, that does not mean you are non-compliant by providing an extension to the basic set of requirements.

I hope we can get the byte arrays support added in the nearest update of spec, e.g. in v1.1, as I believe 8-bit byte buffers telemetry is a fundamental feature of most other telemetry flows. It certainly is also a basic feature in both - Event Tracing for Windows, as well as in LTTNG.

Why future-proofing? Because without it we are going to be trapped in perpetual ABI compatibility hassle, that hassle is unique to C++ SDK, and committee should be able to respect and understand our desire to provide a stable, long-term usable, ABI surface, with no ABI compatibility issues on it. ... extending variant in future rev with new types presents ABI backwards-compatibility challenge. @jsuereth - we discussed it once before.