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

Clarify that lowerCamelCase field names MUST be used for OTLP/JSON #2829

Conversation

tigrannajaryan
Copy link
Member

@tigrannajaryan tigrannajaryan commented Sep 23, 2022

Resolves #2795

This is a breaking change for OTLP/JSON and is allowed because OTLP/JSON is not yet Stable.

@tigrannajaryan
Copy link
Member Author

tigrannajaryan commented Oct 5, 2022

I discussed this with @yurishkuro.

He has a very good argument that we control most of OTLP producers (the SDKs and Collector), while we don't control most of OTLP consumers (e.g. the backends).
In a tradeoff where we need choose between simplicity for producers and and simplicity for consumers we prefer to choose simplicity of consumers at the cost of more complication for producers. We prefer to wear the burden ourselves instead of burdening the consumers.
I agree with this design choice. Because of this I support the following:

  • Producers must only produce lowerCamelCase JSON keys. They must not produce original field names as JSON key. Doing so is trivial, since it is the default behavior of Protobuf JSON marshalers and only requires producers to be careful and avoid choosing a non-default marshalling option. It is also trivial to comply with for producers that don't use Protobuf JSON marshalers.
  • Consumer don't need to worry about accepting original field names. They should only accept lowerCamelCase field names.
    The OTLP/JSON specification will unconditionally call out that lowerCamelCase names are required for JSON keys.

I will update this PR to reflect this thinking and will reset the existing approvals.

@tigrannajaryan tigrannajaryan force-pushed the feature/tigran/clarify-otlp-json-key-names branch from f5a713c to 72df625 Compare October 5, 2022 16:28
@tigrannajaryan tigrannajaryan changed the title Clarify that original and lowerCamelCase field names are allowed for OTLP/JSON Clarify that lowerCamelCase field names MUST be used for OTLP/JSON Oct 5, 2022
@tigrannajaryan tigrannajaryan force-pushed the feature/tigran/clarify-otlp-json-key-names branch 2 times, most recently from 2218e6e to 3be7daa Compare October 5, 2022 16:32
@tigrannajaryan
Copy link
Member Author

Let's give this another 24 hours and then merge if there are no objections.

CHANGELOG.md Outdated Show resolved Hide resolved
Resolves open-telemetry#2795

This is a breaking change for OTLP/JSON and is allowed because OTLP/JSON is not yet Stable.
@tigrannajaryan tigrannajaryan force-pushed the feature/tigran/clarify-otlp-json-key-names branch from ddaeb2b to 2ebe1fd Compare October 12, 2022 16:41
@tigrannajaryan
Copy link
Member Author

Has enough approvals. No objections for several days. Merging.

@tigrannajaryan tigrannajaryan merged commit ac639af into open-telemetry:main Oct 12, 2022
@tigrannajaryan tigrannajaryan deleted the feature/tigran/clarify-otlp-json-key-names branch October 12, 2022 16:45
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.

[otlp] Decide if original and lowerCamelCase JSON keys should be supported
9 participants