opentelemetry-js
opentelemetry-js copied to clipboard
open-telemetry
Support 64bit integers for metrics
The metrics specification calls for two different types of measures: Long and Double. JavaScript has equivalents for these: BigInt and Number respectively, howeverBigInt is still a fairly new addition to ecmascript specification so it isn't supported by older browsers and seems like it was added to node 10.
Rather than having something like
export interface Measure {
createDoubleMeasurement(value: double): Measurement;
createLongMeasurement(value: long): Measurement;
}
should opentelemetry-node only support number? The measure interface would look like
export interface Measure {
createMeasurement(value: number): Measurement;
}
BigInt support has landed on node in 10.4.0 (or 10.8.0 ?) but it's possible to use jsbi as a polyfill in both node and the browser if needed.
I agree with @bg451's approach. Only thing is if they measure type is Long (or Long typed Measure/Measurement.) and provided value with decimals, SDK should either throw an exception or log the error otherwise exporters would crash or throw un handled exceptions.
*this is more like a question/discussion, so removed bug label.
I'd like to avoid making BigInt / jsbi a requirement for users, since it would add weight to the JS bundles that get shipped to the browser.
That said, you can't do fully accurate Long type counting with the regular JS number (though for smaller counts they work pretty well). I would be inclined to focus the UI on number, and make BigInt use either not included initially or an optional add-on of some sort.
There is also the long.js library, which is likely lighter weight than jsbi since it only needs 64-bit support. Again, something I'd recommend we look at down the road.
Some more food for thought from https://github.com/open-telemetry/opentelemetry-js/pull/105/files#diff-1a629a23a3811f06e38fba44580c10bdR42
Measurements can have a long or double type. However, it looks like
// the metric timeseries API (according to spec) accepts values instead of
// Measurements, meaning that if you accept a `number`, the type gets lost.
// Both java and csharp have gone down the route of having two gauge interfaces,
// GaugeDoubleTimeseries and GaugeLongTimeseries, with param for that type. It'd
// be cool to only have a single interface, but maybe having two is necessary?
I guess there's this question of how far we should deviate from the spec, and imo I think this would be a win for API simplicity.
What if we have a single interface, and make the type for it accept something like number|bigint?
Then in our implementation, we make sure that the bigint dependency is pluggable in some way (maybe different packages, one with it, one without it?). In the code we can do a typeof value === 'number' check to determine the type of the input value for whether to use a number or bigint, and the number-only implementation could say write a warning log if typeof value !== 'number'
How will metrics be sent to the server? This is important because some formats like JSON simply don't support Long. There are also certain libraries that expect a specific format from a specific library, like msgpack-lite requires int64-buffer and Protobuf has a similar requirement.
The way the OpenCensus service's grpc-gateway works is that you can give a value field and a string for the number, something like {value: {int64Value: "string of the 64-bit number"}}. For OpenCensus Node it uses gRPC for the OpenCensus service exporter and for the Stackdriver exporter I think. For OpenCensus Web, I pretty much just used double values for all number on export, but would have done something similar with the wrapper object and string.
The value type in the newer specs is either 64 bit double or a 64 bit signed integer (sfixed64, which can be passed to protobufjs as a string). So strictly speaking BigInt can't be supported properly, but it might make sense to support the 64 bit integer API, which in the background uses BigInt, e.g. createInt64Counter.
