Move OTLP spec to github.com/open-telemetry/opentelemetry-proto #457
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Bogdan Drutu <bogdandrutu@gmail.com>
* Add OTLP to the specification We previously had OTLP definition scattered in multiple OTEPs. This amalgamates the OTEPs and brings OTLP to the specification repo. * Address PR comments * Address PR comments
* Allow specifying a single configuration The following change describes that the OTLP exporter must support configuration for all signals via a single set of configuration options. There is also an example for configuring different signals with different endpoints via environment variables. * adding a third example * moving otlp exporter into protocol directory * add link to exporter from readme * apply review feedback
…g (#911) Resolves: open-telemetry/opentelemetry-specification#786 See discussion and motivation for the change in the issue linked above. Co-authored-by: Sergey Kanzhelev <S.Kanzhelev@live.com>
MUST is the correct requirement here. There is no situation when a different content type is a better choice.
* Change default OTLP port number Contributes to open-telemetry/opentelemetry-specification#1148 Note that a separate port is used for OTLP/HTTP for now. There is currently work in progress to confirm that we can use the same port. Once we have the confirmation I will update the spec again to use one port. * Address PR comments
* Remove support to allow_different_nesting, from markdownlint Signed-off-by: Bogdan Drutu <bogdandrutu@gmail.com> * Update specification/metrics/semantic_conventions/README.md Co-authored-by: Armin Ruech <armin.ruech@dynatrace.com> * Run markdown-toc Signed-off-by: Bogdan Drutu <bogdandrutu@gmail.com> Co-authored-by: Armin Ruech <armin.ruech@dynatrace.com>
## Changes It seems wired that the closing tag of `<details>` is missing. As a result, most of the content has been folded. Added a closing `</details>` tags to only fold the table of contents. Before the change:  After the change: 
* Versioning and support based on OTEP 143
Signed-off-by: Bogdan Drutu <bogdandrutu@gmail.com>
…(#1637) Resolves open-telemetry#268 This behavior is already the documented behavior for JSON Protobuf, see https://developers.google.com/protocol-buffers/docs/proto3#json: >JSON value will be a decimal string. Either numbers or strings are accepted.
* Mark sections of datamodel stable. * Generated ToC * Clean up MUST and SHOULD in the document. * adjust for bug in markdown-toc * Mark protocol as stable. Co-authored-by: Carlos Alberto Cortez <calberto.cortez@gmail.com>
The Log SIG discussed and made a decision to declare Log data model and Log part of OTLP Beta. (See SIG meeting notes here https://docs.google.com/document/d/1cX5fWXyWqVVzYHSFUymYUfWxUK5hT97gc23w595LmdM/edit#heading=h.28y76as82bvu)
Related to open-telemetry/opentelemetry-specification#1816 Fixes open-telemetry/opentelemetry-specification#1835 Some historical context: we wanted to make grpc and http use the same port and we had an open issue in the Collector to do so: open-telemetry/opentelemetry-collector#1256 The conclusion is that there are technical hurdles that make it unfeasible: open-telemetry/opentelemetry-collector#1256 (comment) Because of that we need to keep grpc and http ports separate. This means we need to change the spec to say that otlp/http uses port 4318. Once this PR is merged we will also need to submit for port 4318 registration with IANA like we did previously with port 4317: open-telemetry/opentelemetry-specification#1148 (comment)
Apparently there is new evidence that one port may work after all: https://github.com/open-telemetry/opentelemetry-collector/pull/3765/files This reverts commit cce1d5996873de38a68e05eafa4d5e224df9b8f1 until we have the final decision about the ability to have a single port, see: open-telemetry/opentelemetry-specification#1846 (comment)
Related to open-telemetry/opentelemetry-specification#1816 Fixes open-telemetry/opentelemetry-specification#1835 Fixes open-telemetry/opentelemetry-specification#1920 Some historical context: we wanted to make grpc and http use the same port and we had an open issue in the Collector to do so: open-telemetry/opentelemetry-collector#1256 The conclusion is that there are technical hurdles that make it unfeasible: open-telemetry/opentelemetry-collector#1256 (comment) Because of that we need to keep grpc and http ports separate. This means we need to change the spec to say that otlp/http uses port 4318. Once this PR is merged we will also need to submit for port 4318 registration with IANA like we did previously with port 4317: open-telemetry/opentelemetry-specification#1148 (comment) There was also a draft PR to merge the ports in the Collector but it was not completed: open-telemetry/opentelemetry-collector#3765 Note that this change was initially submitted in PR open-telemetry/opentelemetry-specification#1839 and then reverted in PR open-telemetry/opentelemetry-specification#1847 because we hoped that the merging could be successfully done. We believe that we should no longer pursue this and should consider the ports separate from now on. Note 2: we consider this a spec bug fix (spec was impossible to implement), rather than a functionality change, that's why we believe this is an allowed change.
Make Binary and JSON encodings their own sections. This makes the spec easier to understand and also makes adding further clarifications to JSON easier. There are no functional changes. OTLP protocol behavior remains exactly the same. This is purely editorial change.
* Prohibit usage of enum value name strings in OTLP/JSON Resolves open-telemetry#424 This change disallows using enum value names as strings in OTLP/JSON and requires to use enum integer values only. * Fix grammar Co-authored-by: Bogdan Drutu <bogdandrutu@gmail.com>
…(#2816) * Clarify that unknown fields must be ignored when receiving OTLP/JSON Resolves open-telemetry#425 The proposed behavior is necessary for interoperability of senders and receivers when OTLP protocol evolves in an allowed way: by adding new fields to existing messages. * Remove unnecessary sentence * Fix typo Co-authored-by: Yuri Shkuro <yurishkuro@users.noreply.github.com>
See also open-telemetry#436 Co-authored-by: Carlos Alberto Cortez <calberto.cortez@gmail.com>
|
|
|
|
can you illustrate how you copied files with the commits history? |
I produced a patch using git log that only includes commits that touched the relevant files: Then applied the patch in the proto repo: The resulting commit hashes are obviously different, since the patch is applied on top of the current HEAD. I plan to merge this PR without squashing to keep this commit history. If there is a better way to do this, I am happy to try it. |
|
@tigrannajaryan Clever! I've use git filter-branch before to extract out components preserving history, but I never had to merge it with existing history. I think what you did here makes the most sense! |
jsuereth
approved these changes
Apr 24, 2023
jmacd
approved these changes
Apr 25, 2023
djaglowski
approved these changes
Apr 25, 2023
reyang
approved these changes
Apr 25, 2023
@open-telemetry/governance-committee are you OK with overriding the EasyCLA on this PR? From what I see this is a false alarm. The commits in question are from @SergeyKanzhelev @arminru @bogdandrutu and @yurishkuro who all signed the CLA. It would be good if someone else double-checks this. |
carlosalberto
approved these changes
Apr 26, 2023
jack-berg
approved these changes
Apr 26, 2023
jack-berg
reviewed
Apr 26, 2023
bogdandrutu
approved these changes
Apr 26, 2023
arminru
approved these changes
Apr 26, 2023
|
/easycla (trying this as per open-telemetry/community#1445 (comment), cc @trask) |
|
^ @tigrannajaryan CLA is passing now ✅
|
|
I am going to disable "squash" option temporarily to merge this. |
Correction. I enabled "Rebase and Merge" option. If I merge this now, we will have 35 new commits. This will NOT create a merge commit. Anyone thinks we should have a merge commit instead? |
Sounds reasonable to me 👍 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Contributes to open-telemetry/opentelemetry-specification#3415
This PR copies the following files exactly as they are without any modifications:
The PR contains all historic commits that contributed to these files. We need to merge this PR without squashing!
Next steps after this PR is merged: