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

fix otlp grpc exporter bug when no scheme is passed in #1806

Merged
merged 2 commits into from
May 4, 2021

Conversation

codeboten
Copy link
Contributor

Description

When an endpoint without a protocol is passed in, urlparse sets the host as the scheme and leaves the net location empty. This changes catches that and skips setting the endpoint in that case.

Fixes #1801

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)

Does This PR Require a Contrib Repo Change?

  • No.

Checklist:

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

@codeboten codeboten requested a review from a team as a code owner May 3, 2021 16:23
@codeboten codeboten requested review from aabmass and hectorhdzg and removed request for a team May 3, 2021 16:23
Copy link
Contributor

@ocelotl ocelotl left a comment

Choose a reason for hiding this comment

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

Approving, but I think the interface is not intuitive. We have insecure which does not have a default value, if it is not set, then it is defined depending on the protocol or lack of it in the endpoint. So, we have 2 ways of setting insecure, one overrides the other. This is not documented (which is easy to fix), but I wonder if we would be better without this insecure argument at all.

@codeboten
Copy link
Contributor Author

@ocelotl yes I think the insecure parameter should be removed but I didn't want to break backwards compatibility there.

@ocelotl
Copy link
Contributor

ocelotl commented May 3, 2021

@ocelotl yes I think the insecure parameter should be removed but I didn't want to break backwards compatibility there.

Maybe we should ignore the protocol at all, then?

@codeboten
Copy link
Contributor Author

codeboten commented May 3, 2021

Maybe we should ignore the protocol at all, then?

@ocelotl Using the protocol to determine secure/insecure is part of the specification.

@codeboten codeboten merged commit cc51928 into open-telemetry:main May 4, 2021
@codeboten codeboten deleted the codeboten/fix-1801 branch May 4, 2021 18:28
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.

OTLP gRPC exporter silently fails if scheme is not specified in endpoint
3 participants