Skip to content
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

Conflicting requirements in OTLP JSON field names #476

Closed
4 tasks done
Tracked by #456
tigrannajaryan opened this issue Apr 4, 2023 · 14 comments
Closed
4 tasks done
Tracked by #456

Conflicting requirements in OTLP JSON field names #476

tigrannajaryan opened this issue Apr 4, 2023 · 14 comments
Assignees
Labels
bug Something isn't working

Comments

@tigrannajaryan
Copy link
Member

tigrannajaryan commented Apr 4, 2023

Problem

JSON Protobuf Encoding section in OTLP protocol specification says:

The trace_id and span_id byte arrays are represented as ...

Note that trace_id and span_id fields use lower snake case, with underscores, using original field names from the proto.

However, in the same section of the spec we also have this:

The keys of JSON objects are field names converted to lowerCamelCase. Original field names are not valid to use as keys for JSON objects. For example, this is a valid JSON representation of a Resource: { "attributes": {...}, "droppedAttributesCount": 123 }, and this is NOT a valid representation: { "attributes": {...}, "dropped_attributes_count": 123 }.

These 2 clauses conflict with each other. Which one is true? Should we use lower camel case, i.e. traceId/spanId or the original names trace_id/span_id?

We ended up in this broken situation after this PR, where we decided that lowerCamelCase names only must be used.

The spec contradicts with itself and it needs fixing.

Chosen Solution

  • We recognize this a bug that needs to be fixed.
  • We choose camelCase as canonical JSON field casing. Use correct lowerCameCase for JSON field references #468 fixed it in the spec.
  • We make sure Collector's OTLP receiver accepts both capitalizations for one year to give everyone time to transition.
  • All SDK exporters and Collector's OTLP exporter will be immediately updated to use the camelCase (if not already).

Alternate solutions considered

Allow trace_id/span_id names as an exception

We can explicitly call out in the spec that the names of these 2 fields are an exception from the rule about lowerCamelCase. We will end up with a bit of ugliness and inconsistency in the OTLP JSON spec.

If anyone can think of a better solution please comment.

@tigrannajaryan tigrannajaryan added the bug Something isn't working label Apr 4, 2023
@tigrannajaryan
Copy link
Member Author

@open-telemetry/specs-approvers FYI.

@jack-berg
Copy link
Member

The trace_id and span_id byte arrays are represented as case-insensitive hex-encoded strings; they are not base64-encoded as is defined in the standard Protobuf JSON Mapping. Hex encoding is used for trace_id and span_id fields in all OTLP Protobuf messages, e.g., the Span, Link, LogRecord, etc. messages. For example, the trace_id field in a Span can be represented like this: { "trace_id": "5B8EFFF798038103D269B633813FC60C", ... }

The reference to snake case here seems like a typo. This clause is referencing the encoding of the strings, and is not concerns with the casing of keys. I believe the snake case was probably in reference to the field names in the proto definition.

I'm in favor of the first option, but would like to understand more about which components would be disrupted by this.

@tigrannajaryan
Copy link
Member Author

I'm in favor of the first option, but would like to understand more about which components would be disrupted by this.

Any existing sender implementation of OTLP JSON which is compliant with the current spec will be no longer able to send spans to any other receiver implementation of OTLP JSON that is compliant with the modified Otel spec (and vice versa).

This is a catastrophic failure mode for me, the worst kind of a failure to deliver on our stability promise since the only way to avoid being affected by it is co-ordinated updating of all participants (producers and consumers) or a distributed network, which in any non-trivial deployment is either hard or impossible to do.

@jack-berg
Copy link
Member

Any existing sender implementation of OTLP JSON which is compliant with the current spec will be no longer able to send spans to any other receiver implementation of OTLP JSON that is compliant with the modified Otel spec (and vice versa).

Doesn't that assume that a sender producing trace_id / span_id is in fact compliant? As you point out, the language is contradictory. You can make the case that the later bullet point means that sending trace_id / span_id instead of traceId / spanId means the sender is not compliant.

@tigrannajaryan
Copy link
Member Author

tigrannajaryan commented Apr 4, 2023

You are right. It is a matter of which of the contradicting clauses we think was the overriding one and I don't think there is a clear answer to that.

I updated the description of the issue to reflect the fact that depending on what we think was the overriding clause one or the other proposed solution is breaking.

@joaopgrassi
Copy link
Member

joaopgrassi commented May 22, 2023

Does the merge of #468 changes anything in this issue?

@punya
Copy link
Member

punya commented May 22, 2023

Note that the example JSON files in the repository actually use both types of casing in the same file!

  • Snake case: resource_spans, scope_spans
  • Camel case: stringValue, parentSpanId

@tigrannajaryan tigrannajaryan transferred this issue from open-telemetry/opentelemetry-specification May 23, 2023
@tigrannajaryan
Copy link
Member Author

I added this to 1.0 pre-release checklist. We need to resolve this.

@tigrannajaryan
Copy link
Member Author

Does the merge of #468 changes anything in this issue?

Yes, it went with camelCase. But I think there is a bit more work to do before we close this.

@tigrannajaryan
Copy link
Member Author

Note that the example JSON files in the repository actually use both types of casing in the same file!

  • Snake case: resource_spans, scope_spans
  • Camel case: stringValue, parentSpanId

Those are different fields, but I agree they need to be fixed to camelCase too.

@tigrannajaryan
Copy link
Member Author

@open-telemetry/specs-approvers

  • We recognize this a bug that needs to be fixed.
  • We choose camelCase as canonical JSON field casing. Use correct lowerCameCase for JSON field references #468 fixed it in the spec.
  • We make sure Collector's OTLP receiver accepts both capitalizations for one year to give everyone time to transition.
  • All SDK exporters and Collector's OTLP exporter will be immediately updated to use the camelCase (if not already).

Thoughts?

@tigrannajaryan
Copy link
Member Author

@tigrannajaryan
Copy link
Member Author

tigrannajaryan commented May 23, 2023

Also related to this is "Trace Context in non-OTLP Log Formats" document: https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/compatibility/logging_trace_context.md

Should we update it from snake_case to camelCase too?

@open-telemetry/specs-logs-approvers what do you think?

[UPDATE] I created a separate issue for this, let's discuss there: open-telemetry/opentelemetry-specification#3518

tigrannajaryan added a commit to tigrannajaryan/opentelemetry-proto that referenced this issue May 23, 2023
Indirectly related to open-telemetry#476

The spec requires that:

>The keys of JSON objects are field names converted to lowerCamelCase.

See https://github.com/open-telemetry/opentelemetry-proto/blob/main/docs/specification.md#json-protobuf-encoding
@tigrannajaryan
Copy link
Member Author

I opened issues in every language repo to ensure OTLP/JSON exporters use correct capitalization. Closing this issue, there is nothing else to do.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

5 participants