feat(otlp-exporter-http): change otlp-http port to canonical 4318

[secustor]

Signed-off-by: secustor sebastian@poxhofer.at

Which problem is this PR solving?

Used default port is not the one described in the specification for opentelemetry-exporter-otlp-http

Reference: https://github.com/open-telemetry/opentelemetry-specification/blob/401bb9c944323d5e3fc773b88ad3319c01abcff0/specification/protocol/exporter.md

Short description of the changes

This PR modifies the default port of the experimental opentelemetry-exporter-otlp-http exporter from 55681 to the canonical 4318

Type of change

• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• This change requires a documentation update

How Has This Been Tested?

I have run the test cases using npm test

Checklist:

• Followed the style guidelines of this project
• Unit tests have been added
• Documentation has been updated

[rauno56]

Related: #2395

As I understand from the discussions in ☝️, spec intended to stabilize 4317 for the port for OTLP in general? Has it changed?

[secustor]

I have gone initially simply from the current specification, but it seems there has been some discussion.

Speaking from this commit open-telemetry/opentelemetry-specification@90d9aab it seems to be settled on this setup tough.

[rauno56]

IANA is still yet to approve the port reservation.

Although it is in fact following the current spec, I wouldn't rush merging it just yet.

[codecov]

Codecov Report

Merging #2557 (df83311) into main (8bda25c) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #2557   +/-   ##
=======================================
  Coverage   93.22%   93.22%           
=======================================
  Files         157      157           
  Lines        5432     5432           
  Branches     1141     1141           
=======================================
  Hits         5064     5064           
  Misses        368      368           

[vmarchaud]

We definively had some trouble in the past with the back and forth to use one port for both http and grpc so i would like to wait to have a definitive spec to follow before merging this (i guess after IANA will approve the port)

[github-actions]

This PR is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 14 days.

[dyladan]

This PR has many conflicts, but the new collector has probably been out long enough to use the new default port

[dyladan]

According to https://github.com/open-telemetry/opentelemetry-specification/pull/1970/files it looks like the default should also be https?

[secustor]

Not sure if it is wise to change this, as this could break setups. It is probably the best to wait till a x.0.0 release and change it then.

[dyladan]

According to open-telemetry/opentelemetry-specification#1970 (files) it looks like the default should also be https?

doh!

I agree we can't really change it now but it is unfortunate.

[dyladan]

According to open-telemetry/opentelemetry-specification#1970 (files) it looks like the default should also be https?

doh!

I agree we can't really change it now but it is unfortunate.

Actually because we haven't released the exporter yet as 1.x we can change it. I think we should try to follow spec as closely as possible which would mean using http://localhost:4318 as the default for HTTP and http://localhost:4317 for gRPC. Looks like the default was changed from https to http about 3 months ago: open-telemetry/opentelemetry-specification#2014

I do question why use http:// for gRPC instead of grpc:// but that might just be a minor spec inconsistency that we can ignore for now safely.

[dyladan]

@secustor can you address @blumamir's comments so we can merge this PR? We want to release the 1.0 of the package and this change is one we want in before we can release.

[secustor]

I have updated all components of those two example to the newest versions and fixed the breaking changes