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.

Sign up for GitHub

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: Handle error case when no credentials supplied #1366

Merged
merged 7 commits into from
Nov 18, 2020
Merged

OTLP exporter: Handle error case when no credentials supplied #1366

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
0541b35
Handle credential not set
jan25 Nov 9, 2020
4a4d8ed
Test
jan25 Nov 9, 2020
800f6b2
Revert unrelated change
jan25 Nov 9, 2020
b97112f
Update CHANGELOG.md
jan25 Nov 11, 2020
32544f4
Merge branch 'master' into jan25/issue-1004-bug
lzchen Nov 12, 2020
367d0d1
Merge branch 'master' into jan25/issue-1004-bug
lzchen Nov 17, 2020
b7b086d
Merge branch 'master' into jan25/issue-1004-bug
lzchen Nov 18, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions exporter/opentelemetry-exporter-otlp/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@

- Add Gzip compression for exporter
([#1141](https://github.com/open-telemetry/opentelemetry-python/pull/1141))
- OTLP exporter: Handle error case when no credentials supplied
([#1366](https://github.com/open-telemetry/opentelemetry-python/pull/1366))
## Version 0.15b0

Released 2020-11-02
Expand Down
2 changes: 1 addition & 1 deletion exporter/opentelemetry-exporter-otlp/src/opentelemetry/exporter/otlp/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@
trace.set_tracer_provider(TracerProvider(resource=resource)))
tracer = trace.get_tracer(__name__)

otlp_exporter = OTLPSpanExporter(endpoint="localhost:55680")
otlp_exporter = OTLPSpanExporter(endpoint="localhost:55680", insecure=True)

span_processor = BatchExportSpanProcessor(otlp_exporter)

Expand Down
24 changes: 16 additions & 8 deletions exporter/opentelemetry-exporter-otlp/src/opentelemetry/exporter/otlp/exporter.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -201,15 +201,23 @@ def __init__(
self._client = self._stub(
insecure_channel(endpoint, compression=compression_algorithm)
)
else:
credentials = credentials or _load_credential_from_file(
Configuration().EXPORTER_OTLP_CERTIFICATE
)
self._client = self._stub(
secure_channel(
endpoint, credentials, compression=compression_algorithm
)
return

# secure mode
if (
credentials is None
and Configuration().EXPORTER_OTLP_CERTIFICATE is None
):
raise ValueError("No credentials set in secure mode.")
Copy link
Contributor

Choose a reason for hiding this comment

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

any advantage to raising an exception here and not default to using grpc.ssl_channel_credentials()?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This case is when insecure=False which is by default. Without this custom error handling it would break deep in stack inside grpc library without a useful message. Or can we handle this in a better way?

Copy link
Member

Choose a reason for hiding this comment

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

It's probably reasonable. I don't know how bad the original error is but asserting invalid configurations is certainly helpful.

Copy link
Contributor

Choose a reason for hiding this comment

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

@jan25 my suggestion here was that using ssl_channel_credentials without any parameters would work in the case of a secure connection as it will load the root certificate from a default location:
https://grpc.github.io/grpc/python/grpc.html#create-client-credentials

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like this idea, perhaps we need to update the spec before using default location? Also, i was wrong about grpc library erroring. Actually it was open(filename) thats erroring when no cert location is set and filename=None.

Copy link
Member

Choose a reason for hiding this comment

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

@jan25 if it violates the spec to look for ssl credentials in the secure case before failing, then agreed we should amend the spec.

Can you file an issue? Or do you want someone else to handle that conversation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @toumorokoshi I recently learnt grpc python library has the default certificate path support, so i don't have 100% context of this. Could someone else open an issue in spec repo about this?


credentials = credentials or _load_credential_from_file(
Configuration().EXPORTER_OTLP_CERTIFICATE
)
self._client = self._stub(
secure_channel(
endpoint, credentials, compression=compression_algorithm
)
)

@abstractmethod
def _translate_data(
Expand Down
4 changes: 4 additions & 0 deletions exporter/opentelemetry-exporter-otlp/tests/test_otlp_metric_exporter.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,10 @@ def test_env_variables(self, mock_exporter_mixin):
self.assertIsNotNone(kwargs["credentials"])
self.assertIsInstance(kwargs["credentials"], ChannelCredentials)

def test_no_credentials_error(self):
with self.assertRaises(ValueError):
OTLPMetricsExporter()

@patch("opentelemetry.sdk.metrics.export.aggregate.time_ns")
def test_translate_metrics(self, mock_time_ns):
# pylint: disable=no-member
Expand Down
4 changes: 4 additions & 0 deletions exporter/opentelemetry-exporter-otlp/tests/test_otlp_trace_exporter.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -183,6 +183,10 @@ def test_env_variables(self, mock_exporter_mixin):
self.assertIsNotNone(kwargs["credentials"])
self.assertIsInstance(kwargs["credentials"], ChannelCredentials)

def test_no_credentials_error(self):
with self.assertRaises(ValueError):
OTLPSpanExporter()

@patch("opentelemetry.exporter.otlp.exporter.expo")
@patch("opentelemetry.exporter.otlp.exporter.sleep")
def test_unavailable(self, mock_sleep, mock_expo):
Expand Down