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

OTLP exporter uses scheme from endpoint configuration #1771

Merged
merged 11 commits into from
Apr 16, 2021

Conversation

codeboten
Copy link
Contributor

Description

As per the specification, the otlp exporter must use the scheme from the endpoint to determine if a connection should be secure or not. https://github.com/open-telemetry/opentelemetry-specification/blob/f62744a679814937214fd17394ab3fa8a9099424/specification/protocol/exporter.md#configuration-options

A scheme of https indicates a secure connection.

Fixes #1747

Type of change

Please delete options that are not relevant.

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

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

  • Ran manual tests w/ the collector locally

Does This PR Require a Contrib Repo Change?

  • No.

Checklist:

  • Followed the style guidelines of this project
  • Changelogs have been updated

@codeboten codeboten requested review from a team, aabmass and lzchen and removed request for a team April 14, 2021 15:37
Copy link
Contributor

@owais owais left a comment

Choose a reason for hiding this comment

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

LGTM. Wondering if using urlparse would be better?

@codeboten codeboten changed the title initial commit for fix for issue 1747 OTLP exporter uses scheme from endpoint configuration Apr 14, 2021
@ocelotl
Copy link
Contributor

ocelotl commented Apr 14, 2021

LGTM. Wondering if using urlparse would be better?

Yes, please, urlparse is part of the standard library:

>>> from urllib.parse import urlparse
>>> parse_result = urlparse("http://localhost:4317")
>>> parse_result.scheme
'http'
>>> parse_result.hostname
'localhost'
>>> parse_result.port
4317
>>> parse_result.netloc
'localhost:4317'

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 since this fixes the issue, but please consider using urlparse, @codeboten ✌️

@codeboten
Copy link
Contributor Author

Updated the code to use urlparse @owais @ocelotl

)

parsed_url = urlparse(endpoint)

if endpoint is None:
Copy link
Member

Choose a reason for hiding this comment

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

did you intend to write is not here? Because there is a default value and I don't see how it can be None? Could you also add a test to insecure_channel?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thank you for catching this, i meant to check for insecure, there's already a few tests for the insecure_channel but i can add one specifically with the endpoint parameter.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

test added if you can take a look @lonewolf3739

Copy link
Member

@srikanthccv srikanthccv left a comment

Choose a reason for hiding this comment

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

LGTM

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.

Exporting traces to insecure gRPC endpoint with auto instrumentation
5 participants