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

docs(otlp-grpc-exporter): update default url #2726

Merged

Conversation

svetlanabrennan
Copy link
Contributor

@svetlanabrennan svetlanabrennan commented Jan 20, 2022

Signed-off-by: Svetlana Brennan svetlana.svn@gmail.com

Which problem is this PR solving?

Fix readme to mention that default url is scheme-less.

Short description of the changes

Type of change

Please delete options that are not relevant.

I don't think this update requires additional doc updates.

How Has This Been Tested?

None needed

Checklist:

  • Followed the style guidelines of this project
  • Documentation has been updated

Signed-off-by: Svetlana Brennan <svetlana.svn@gmail.com>
@svetlanabrennan svetlanabrennan requested a review from a team January 20, 2022 21:21
@codecov
Copy link

codecov bot commented Jan 20, 2022

Codecov Report

Merging #2726 (6f223f5) into main (31380ac) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #2726   +/-   ##
=======================================
  Coverage   93.27%   93.27%           
=======================================
  Files         159      159           
  Lines        5445     5445           
  Branches     1143     1143           
=======================================
  Hits         5079     5079           
  Misses        366      366           

Signed-off-by: Svetlana Brennan <svetlana.svn@gmail.com>
…ennan/opentelemetry-js into fix-oltp-grpc-exporter-readme
…ennan/opentelemetry-js into fix-oltp-grpc-exporter-readme
Copy link
Member

@dyladan dyladan left a comment

Choose a reason for hiding this comment

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

I think the exporter will not add https. If anything it would be http because the default is not to use secure requests. Will it add any scheme at all?

@svetlanabrennan
Copy link
Contributor Author

My understanding that the exporter will set "https" if a scheme isn't already provided with the url is based on this code.

I'm still new to this so please correct me if my understanding is wrong. :)

@dyladan
Copy link
Member

dyladan commented Jan 25, 2022

Looks like your docs are correct but the code is wrong :)

The specification states the default should be http. Honestly in this particular case it doesn't matter because grpc doesn't use the scheme anyway so the information ends up being thrown away.

@svetlanabrennan
Copy link
Contributor Author

Got it. So you want t leave it as "https" since it's not used anyway?

@dyladan
Copy link
Member

dyladan commented Jan 31, 2022

I would just remove the "if the provided url is schemeless..." lines

@svetlanabrennan
Copy link
Contributor Author

svetlanabrennan commented Jan 31, 2022

I've removed the lines mentioning "if the provided url is schemeless...".

@vmarchaud vmarchaud added the document Documentation-related label Feb 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
document Documentation-related
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants