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

Update OTLP/HTTP Default Port Per Changes to the Spec #438

Merged
merged 5 commits into from Oct 11, 2021

Conversation

Grunet
Copy link
Contributor

@Grunet Grunet commented Oct 7, 2021

Resolves #431

My biggest concern has to do with the 2nd commit, in particular b/c I couldn't find evidence in the spec of a non-protobuf-ed HTTP transport mechanism, which it seemed like Contrib/Otlp/Exporter.php is trying to do (sending an ordinary HTTP request, maybe with a JSON body). So I wasn't sure if updating the default port was appropriate in this case.

A smaller concern also has to do with the 2nd commit, namely if updating the port on the collector container in the compose file was OK (seemed to me like it was per this PR from the collector repo)

@codecov
Copy link

codecov bot commented Oct 7, 2021

Codecov Report

Merging #438 (e17a242) into main (8d61da2) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##               main     #438   +/-   ##
=========================================
  Coverage     91.14%   91.14%           
  Complexity      804      804           
=========================================
  Files            76       76           
  Lines          1977     1977           
=========================================
  Hits           1802     1802           
  Misses          175      175           
Impacted Files Coverage Δ
src/Contrib/OtlpHttp/Exporter.php 97.14% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8d61da2...e17a242. Read the comment docs.

@Grunet
Copy link
Contributor Author

Grunet commented Oct 7, 2021

Not sure I quite understand why the first codecov/project check failed, but may be missing something subtle

@SeanHood
Copy link
Contributor

SeanHood commented Oct 7, 2021

For the 2nd commit, src/Contrib/Otlp/Exporter.php is no longer in use. I think this was a previous attempt at OTLP. This exporter should be removed tbh.

src/Contrib/OtlpHttp/Exporter.php is the working HTTP Exporter which uses the supported Protobuf format.

As for the docker-compose update, looks good to me.

@Grunet
Copy link
Contributor Author

Grunet commented Oct 8, 2021

For the 2nd commit, src/Contrib/Otlp/Exporter.php is no longer in use. I think this was a previous attempt at OTLP. This exporter should be removed tbh.

src/Contrib/OtlpHttp/Exporter.php is the working HTTP Exporter which uses the supported Protobuf format.

Saw (late) this was already being taken care of in #440 so I removed the commit I had for it from this PR

@bobstrecansky
Copy link
Collaborator

My apologies @Grunet - I thought you had force pushed Sean’s changes, but they had not quite made it into main yet - you should be able to now.

@Grunet
Copy link
Contributor Author

Grunet commented Oct 10, 2021

@bobstrecansky I saw Sean's changes were merged into main so I brought all of main's changes into my branch now (was having trouble rebasing so merged instead)

@bobstrecansky bobstrecansky merged commit ef2ccc4 into open-telemetry:main Oct 11, 2021
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.

Use 4318 as the default OLTP/HTTP port
3 participants