Skip to content

fix(otlp_output): parse RFC 7951 numeric JSON strings as metric values - #893

Open
Mesverrum wants to merge 2 commits into
openconfig:mainfrom
Mesverrum:fix/otlp-numeric-json-ietf-strings
Open

Mesverrum wants to merge 2 commits into
openconfig:mainfrom
Mesverrum:fix/otlp-numeric-json-ietf-strings

Conversation

@Mesverrum

Copy link
Copy Markdown

This change aligns the OTEL output with the way the Prometheus output works.

json_ietf encodes YANG uint64/int64/decimal64 as JSON strings. The OTLP converter previously treated every string as an enum (value=1 + value attribute), causing wrong Prom values and label cardinality explosion. Prefer numeric datapoints when the string parses, matching prometheus_output.

json_ietf encodes YANG uint64/int64/decimal64 as JSON strings. The OTLP
converter previously treated every string as an enum (value=1 + value
attribute), causing wrong Prom values and label cardinality explosion.
Prefer numeric datapoints when the string parses, matching prometheus_output.

@karimra karimra left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this. The direction is correct but it needs a small change:
The change bypasses strings-as-attributes. The default for is false, that means string values are dropped. With this change, any string that parses as a number becomes a data point, even when the option is false. So users on the default config will see new metrics that did not exist before.

I think the numeric check should run after the strings-as-attributes gate, or use its own option.

@Mesverrum

Copy link
Copy Markdown
Author

Thanks for taking a look. I went back and forth on this one and I think the gate order is actually the whole bug, so I want to lay out why before I change it.

The prometheus output doesn't have the gate problem. toFloat in prometheus_common.go runs ParseFloat on every string first and only the ones that fail the parse get dropped or turned into a label depending on strings-as-labels. So anyone on json_ietf with the prom output has been getting in-octets and friends as real samples all along. The OTLP converter checks strings-as-attributes first, and since the default is false the string is dropped before it ever reaches createNumberDataPointWithValue, which already has a ParseFloat case sitting in it that nothing can hit. RFC 7951 puts uint64/int64/decimal64 on the wire as JSON strings, so on the default config every 64 bit counter from a json_ietf subscription just disappears, and with the flag on you get value=1 and the counter stuffed into a value attribute instead.

If the numeric check moves behind the gate, the default config stays broken. It only fixes the people who already turned the flag on, which is the config that also turns every enum into a label. So that version doesn't really fix the thing I hit.

You're right that this is a behavior change for defaults though. People on json_ietf with the flag off will start seeing series they don't have today. I'd argue that's the fix rather than a side effect, and it makes OTLP match what prom already does, but I'm happy to call it out as a behavior change in the changelog and docs.

If you'd still rather have it behind its own option I can add one, something like parse-numeric-strings, but I'd want it defaulting to true. Defaulting it off just keeps the current silence and makes people discover the problem the same way I did.

One limit worth having on the record either way: once the value has been through json.Unmarshal there's no type left on it, so a YANG string leaf whose text happens to be "100" will become a metric too. Prom has that same collision today and the only real fix is checking the leaf type against a loaded model, which is a different change.

Let me know which way you want it and I'll update the PR.

…rings

The review asked for a dedicated option so numeric JSON strings do not
bypass strings-as-attributes. The option defaults to true, so json_ietf
uint64/int64/decimal64 stay numeric samples on the default config. Set
it false to restore the previous drop-or-attribute behavior.

Co-authored-by: Cursor <cursoragent@cursor.com>
@Mesverrum

Copy link
Copy Markdown
Author

Updated with a dedicated option, parse-numeric-strings, defaulting to true.

Putting the numeric check behind strings-as-attributes would leave the default config broken. RFC 7951 still encodes uint64/int64/decimal64 as JSON strings, and with that flag off those counters never reach the parser. This option is the separate gate. Default true matches prometheus_output, which runs toFloat before strings-as-labels. Set it to false and every string goes only through strings-as-attributes again: dropped, or value=1 plus a value attribute.

Docs call out the behavior change for existing json_ietf users, and the remaining limit: a YANG string leaf whose text happens to be numeric is also exported as a number, same as the Prometheus output. Distinguishing that needs the leaf type from a loaded model.

go test ./pkg/outputs/otlp_output/ passes.

This branch has not been deployed

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants