-
Notifications
You must be signed in to change notification settings - Fork 303
examples/metrics.json: Represent uint64 and fixed64 fields as strings #748
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
Protobuf spec says:
Looks like unnecessary change to me unless I am misreading the spec. |
|
Hi @tigrannajaryan By looking at this example JSON, one would assume that it will match with the output of the above function. |
|
Representing counts as strings seems like an issue that needs fixing in the collector, not in this example. |
|
@dmathieu |
|
Sorry, I was wrong. There is no issue in the collector. JSON can't handle int64s, as some serializers wouldn't be able to parse them properly. The current example is still valid though, since the proto specification here accepts both strings and integers. If you intent to ingest OTLP, you will therefore need to be able to receive both (the collector is not the only emitter of OTLP data). |
|
@tigrannajaryan @dmathieu |
|
Yes. But your comment assumes |
|
Shouldn't any SDK follow the Proto spec and output string "only" ? |
|
Hi @tigrannajaryan, @dmathieu |
What was changed?
Updated the datatypes of fixed64, uint64 proto fields in the (Proto)JSON example to string
Modified fields
uint64 fields
ExponentialHistogramDataPoint.Buckets.bucket_counts
fixed64 fields:
ExponentialHistogramDataPoint.count
ExponentialHistogramDataPoint.zero_count
HistogramDataPoint.count
HistogramDataPoint.bucket_counts
Why?
As per specification.md - JSON Protobuf Encoding
As per ProtoJSON format - Representation of each type,
fixed64,uint64proto fields are represented as strings in ProtoJSONJustification
Either numbers or strings are accepted,pmetric.JSONMarshaler.MarshalMetrics()(ref) always generates the JSON with these datatypes as strings.This may lead to confusion.
To avoid this, it's better to maintain example JSON with string datatype so that while it can be used as input, it could also be compared with output without any confusion.
fixed64fields likestart_time_unix_nanoandtime_unix_nanoare already represented as strings, making the notation inconsistent within this example. This only adds to the confusion.Testing
The datapoints were sent to otel collector which is configured to receive the metric and log it.
Payload
{ "resourceMetrics": [ { "resource": { "attributes": [ { "key": "service.name", "value": { "stringValue": "my.service" } } ] }, "scopeMetrics": [ { "scope": { "name": "my.library", "version": "1.0.0", "attributes": [ { "key": "my.scope.attribute", "value": { "stringValue": "some scope attribute" } } ] }, "metrics": [ { "name": "my.histogram", "unit": "1", "description": "I am a Histogram", "histogram": { "aggregationTemporality": 1, "dataPoints": [ { "startTimeUnixNano": "1544712660300000000", "timeUnixNano": "1544712660300000000", "count": "2", "sum": 2, "bucketCounts": ["1","1"], "explicitBounds": [1], "min": 0, "max": 2, "attributes": [ { "key": "my.histogram.attr", "value": { "stringValue": "some value" } } ] } ] } }, { "name": "my.exponential.histogram", "unit": "1", "description": "I am an Exponential Histogram", "exponentialHistogram": { "aggregationTemporality": 1, "dataPoints": [ { "startTimeUnixNano": "1544712660300000000", "timeUnixNano": "1544712660300000000", "count": "3", "sum": 10, "scale": 0, "zeroCount": "1", "positive": { "offset": 1, "bucketCounts": ["0","2"] }, "min": 0, "max": 5, "zeroThreshold": 0, "attributes": [ { "key": "my.exponential.histogram.attr", "value": { "stringValue": "some value" } } ] } ] } } ] } ] } ] }Log
Otel collector is able to read the fields as expected and the log is as follows.