Change OTLP/HTTP port from 4317 to 4318 (#1839) #1970
Conversation
Related to open-telemetry#1816 Fixes open-telemetry#1835 Fixes open-telemetry#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#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#1839 and then reverted in PR open-telemetry#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.
tigrannajaryan
force-pushed
the
separate-otlphttp-port
branch
from
f2926c7 to
512ec14
Compare
|
@open-telemetry/specs-approvers please review this PR with a long history of back-and-forth changes. Hopefully this is the last time we touch the port. |
|
@open-telemetry/specs-approvers I think we should include this as part of 1.7 to avoid further confusion. Sounds about right? |
Not for Collector, because Collector already uses separate ports 4317 and 4318 for a while now. It may be a breaking change for a receiver that expects to receive OTLP/HTTP on port 4317. However I am not aware of any existing implementation that does and given that we met difficulties when trying to have one port on the receiver side it is very likely that no such receivers exist at all. So, even if it is a breaking change the impact is likely very small and fixing it should be easy (just set an env var on the SDK side to use a different port). From formal perspective, yes, it is a breaking change because the spec says OTLP/HTTP port is 4318 and it is a Stable section: https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/protocol/otlp.md#otlphttp However, since we are unable to implement in the Collector what the spec says I think we can treat this as a spec bug, something that is simply wrong and needs to be fixed in the spec. This makes it an allowed change, we are supposed to fix spec bugs even if they mean someone who relied on the bug may be broken as a result of the change. |
|
@open-telemetry/specs-approvers I will keep this open for one more day at least. If there are any doubts please speak up otherwise I will merge and after that will be strongly opposed to reversing/reverting this decision again. |
|
FWIW, |
|
@tigrannajaryan Are we really to roll? Or you want to wait a little longer? |
2 more hours to complete 24 hours since I promised at least one more day :-) |
tigrannajaryan
added
the
spec:protocol
|
6 approvals, 2 days since last changed, I believe this can be merged. |
|
@mtwo please file port 4318 with IANA (I believe you know how it's done since you did it last time). |
|
Filed, ticket number is 1210263 |
|
Update: my request to extend our existing reservation to the adjacent port was declined, as new port reservations require a new submission. I've since filed submission 1213852 with the IANA and I will reply here when I hear back. |
|
Update: IANA has rejected our submission on the basis that we should use the same port for HTTP and gRPC. I'm adding Tigran to the thread. |
|
I replied to IANA with our justification. |
|
@tigrannajaryan any update on the IANA submission would be very appreciated. |
|
IANA rejected the request since they don't believe the justification we provided warrants another port registration. The new port will remain unregistered for now until we find stronger arguments to ask for the new port. |
|
FWIW I completely agree with IANA's decision. HTTP is HTTP regardless of whether the application decides to send gRPC frames or not. |
@anuraaga Can you help make it happen in the Collector's OTLP receiver? We tried and were unable. |
|
Maybe, but I am wondering why as the creators of both gRPC and OpenTelemetry (via OC), Google isn't stepping up to take responsibility and fix the mess. |
…elemetry#1970) Related to open-telemetry#1816 Fixes open-telemetry#1835 Fixes open-telemetry#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#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#1839 and then reverted in PR open-telemetry#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.
Related to #1816
Fixes #1835
Fixes #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:
#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 #1839
and then reverted in PR #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.