-
Notifications
You must be signed in to change notification settings - Fork 821
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
docs(otlp-grpc-exporter): update default url #2726
Conversation
Signed-off-by: Svetlana Brennan <[email protected]>
Codecov Report
@@ 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 <[email protected]>
…ennan/opentelemetry-js into fix-oltp-grpc-exporter-readme
…ennan/opentelemetry-js into fix-oltp-grpc-exporter-readme
There was a problem hiding this 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?
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. :) |
Looks like your docs are correct but the code is wrong :) The specification states the default should be |
Got it. So you want t leave it as "https" since it's not used anyway? |
I would just remove the "if the provided url is schemeless..." lines |
Signed-off-by: Svetlana Brennan <[email protected]>
…ennan/opentelemetry-js into fix-oltp-grpc-exporter-readme
I've removed the lines mentioning "if the provided url is schemeless...". |
Signed-off-by: Svetlana Brennan [email protected]
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: