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

Follow spec defined port 4318 for HTTP exporter #2395

Closed

Conversation

NathanielRN
Copy link
Contributor

Which problem is this PR solving?

Follow up to #2331 , the specifications just updated the HTTP port to be 4318 in this PR: open-telemetry/opentelemetry-specification#1839

Likewise, the Collector also plans to updated from 55681 to 4318, but will support both for some time.

Short description of the changes

  • Updates port for Metrics/Trace HTTP exporter to 4318

@codecov
Copy link

codecov bot commented Aug 3, 2021

Codecov Report

Merging #2395 (4dc3b49) into main (e1b3e98) will not change coverage.
The diff coverage is n/a.

❗ Current head 4dc3b49 differs from pull request most recent head 085ed57. Consider uploading reports for the commit 085ed57 to get more accurate results

@@           Coverage Diff           @@
##             main    #2395   +/-   ##
=======================================
  Coverage   92.78%   92.78%           
=======================================
  Files         145      145           
  Lines        5226     5226           
  Branches     1071     1071           
=======================================
  Hits         4849     4849           
  Misses        377      377           

@dyladan
Copy link
Member

dyladan commented Aug 4, 2021

Should we hold off on this until the collector itself is updated?

@NathanielRN
Copy link
Contributor Author

Looks like the collector PR to change the port has been merged. In terms of releases it looks like the collector does monthly releases in the first week on the month so the August one should be here soon?

As long as users update their collector version quickly it won't be a problem. But we could be safe and let this sit for a while since the collector will still support the old 55681 port for a little bit.

Copy link
Member

@obecny obecny left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in general looks fine, but in the example we are still using version 0.25 which has been released in April - 4 months ago so I can assume that the example will not work anymore ?

@obecny
Copy link
Member

obecny commented Aug 26, 2021

@NathanielRN friendly ping

@longility
Copy link
Contributor

Just a heads up that port number may still be in flux.

Merged reverted PR: open-telemetry/opentelemetry-specification#1847

The draft PR with possible upcoming change: open-telemetry/opentelemetry-collector#3765

I’m sharing as I stumbled upon and seems related, but I’m not sure how it impacts all this. I’ll let y’all decide.

@dyladan
Copy link
Member

dyladan commented Aug 30, 2021

Just a heads up that port number may still be in flux.

Merged reverted PR: open-telemetry/opentelemetry-specification#1847

The draft PR with possible upcoming change: open-telemetry/opentelemetry-collector#3765

I’m sharing as I stumbled upon and seems related, but I’m not sure how it impacts all this. I’ll let y’all decide.

Thanks for the heads up. We've been moving slowly with this change intentionally since we know the collector will continue to support the existing port for quite a long time so we have no real reason to force this change through until the dust is settled.

@NathanielRN
Copy link
Contributor Author

I'll close this PR, the call out about the collector going back to 1 port was really useful! Agree that we can update when the dust has settled, and the old port will continue to work for a while!

@dyladan
Copy link
Member

dyladan commented Sep 14, 2021

What do you mean going back to 1 port?

@NathanielRN
Copy link
Contributor Author

NathanielRN commented Sep 14, 2021

@dyladan in open-telemetry/opentelemetry-specification#1847 which @longility posted above they said

Apparently there is new evidence that one port may work after all:
https://github.com/open-telemetry/opentelemetry-collector/pull/3765/files

Originally they had said they need to use port 4137 to receive gRPC data and port 4138 to receive HTTP data. But I guess they found a way to use 1 port for both these data types?

@NathanielRN NathanielRN deleted the spec-http-port-4318 branch September 14, 2021 19:47
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.

None yet

4 participants