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

Add Env variables in OTLP exporter #1101

Merged
merged 36 commits into from
Oct 28, 2020
Merged

Conversation

jan25
Copy link
Contributor

@jan25 jan25 commented Sep 13, 2020

Description

Adds support for Env variables in OTLP trace and metrics exporter. Implemented subset of variables from exporter spec, other variables will be implemented as the usecases are added to otlp exporter.

Fixes #1004 (issue)

Type of change

  • New feature (non-breaking change which adds functionality)
  • This change requires a documentation update

How Has This Been Tested?

  • Write unit tests

Checklist:

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

@jan25 jan25 requested a review from a team September 13, 2020 11:00
@codeboten codeboten added the release:required-for-ga To be resolved before GA release label Sep 17, 2020
@jan25 jan25 changed the title WIP: Add Env variables in OTLP exporter Add Env variables in OTLP exporter Sep 21, 2020
@jan25
Copy link
Contributor Author

jan25 commented Sep 21, 2020

We need to wait for this PR for spec updates, so we follow the correct default for OTEL_.._INSECURE env variable.

Also, for PR checklist:

  • I'll update readme file for exporter to mention about env variables
  • Unit tests could be tricky to implement, my idea is to start a grpc server in unit test with tls enabled and listening to specific endpoint/port and client sends a sample span to the server through using env vars. Any suggestions here?

@lzchen
Copy link
Contributor

lzchen commented Sep 21, 2020

@jan25
Can you add an entry in the CHANGELOG?

Unit tests could be tricky to implement, my idea is to start a grpc server in unit test with tls enabled and listening to specific endpoint/port and client sends a sample span to the server through using env vars. Any suggestions here?

This seems like an integration test. For a unit test, you simply need to test the constructor populates the desired fields for each use case, as well as your file opening logic works as expected for credentials.

@jan25 jan25 changed the title Add Env variables in OTLP exporter [WIP] Add Env variables in OTLP exporter Sep 21, 2020
@codeboten
Copy link
Contributor

@jan25 is this still a WIP?

@jan25
Copy link
Contributor Author

jan25 commented Oct 1, 2020

@codeboten yes, I'll add unit tests and unmark WIP. Also, need to revert small changes i made assuming exporter changes will be merged.

@jan25 jan25 changed the title [WIP] Add Env variables in OTLP exporter Add Env variables in OTLP exporter Oct 11, 2020
credentials: ChannelCredentials object for server authentication
metadata: Metadata to send when exporting
"""

def __init__(
self,
endpoint: str = "localhost:55680",
endpoint: Optional[str] = None,
insecure: Optional[bool] = None,
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing protocol, certificate_file in constructor, compression and timeout?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could add env vars for protocol compression when those usecases are implemented. Currently exporter uses grpc only and no implementation for compression.

certificate_file is wrapped in ChannelCredentials object which is passed down from OTLPSpanExporter|MetricExporter. I'll change timeout logic to use env var too.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

@lzchen lzchen left a comment

Choose a reason for hiding this comment

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

Some comments.

jan25 and others added 8 commits October 16, 2020 11:52
Signed-off-by: Abhilash Gnan <[email protected]>
Signed-off-by: Abhilash Gnan <[email protected]>
Signed-off-by: Abhilash Gnan <[email protected]>
Signed-off-by: Abhilash Gnan <[email protected]>
Signed-off-by: Abhilash Gnan <[email protected]>
Signed-off-by: Abhilash Gnan <[email protected]>
Signed-off-by: Abhilash Gnan <[email protected]>
@codeboten
Copy link
Contributor

@jan25 looks like some tests are still failing, is the PR still a work in progress or is it ready for review?

@jan25
Copy link
Contributor Author

jan25 commented Oct 23, 2020

@codeboten Tests are failing on older Python versions due to unittest api changes, I've tried a few things but didn't succeed, I'll have to fix them. Otherwise this is ready to review

Copy link
Contributor

@codeboten codeboten left a comment

Choose a reason for hiding this comment

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

Overall this looks pretty good! Just a request for change on the changelog. It would be good have information about the exporter environment variables in docs somewhere

- Update OpenTelemetry protos to v0.5.0
([#1143](https://github.com/open-telemetry/opentelemetry-python/pull/1143))
- Add Env variables in OTLP exporter
([#1101](https://github.com/open-telemetry/opentelemetry-python/pull/1101))
Copy link
Contributor

Choose a reason for hiding this comment

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

this should be moved to unreleased

Signed-off-by: Abhilash Gnan <[email protected]>
Signed-off-by: Abhilash Gnan <[email protected]>
Signed-off-by: Abhilash Gnan <[email protected]>
@jan25
Copy link
Contributor Author

jan25 commented Oct 27, 2020

@codeboten Thanks! I've fixed the tests and updated changelog too.

Do you suggest we add a link to exporter-spec in README.rst file? Perhaps we can also add about whats currently implemented?

@codeboten
Copy link
Contributor

Do you suggest we add a link to exporter-spec in README.rst file? Perhaps we can also add about whats currently implemented?

I added a note to capture this work in the issue to review documentation, I don't want to block this PR on it and there are a bunch of places we need to review env variables anyways #1133 (comment)

Copy link
Contributor

@codeboten codeboten left a comment

Choose a reason for hiding this comment

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

PR looks great!

@codeboten codeboten merged commit 42abfb7 into open-telemetry:master Oct 28, 2020
@jan25 jan25 deleted the issue-1004 branch October 28, 2020 20:21
Copy link
Contributor

@NathanielRN NathanielRN left a comment

Choose a reason for hiding this comment

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

This works, just leaving my comments about some UX issues I had today.

@@ -113,6 +116,16 @@ def _get_resource_data(
return resource_data


def _load_credential_from_file(filepath) -> ChannelCredentials:
try:
with open(filepath, "rb") as f:
Copy link
Contributor

@NathanielRN NathanielRN Oct 30, 2020

Choose a reason for hiding this comment

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

Tried to use the OTLP Exporter today and I think I ran into an issue. If you don't have the OTEL_PYTHON_EXPORTER_OTLP_CERTIFICATE Env variable set, then the call to Configuration().EXPORTER_OTLP_CERTIFICATE

https://github.com/open-telemetry/opentelemetry-python/blob/master/exporter/opentelemetry-exporter-otlp/src/opentelemetry/exporter/otlp/exporter.py#L175

will return None instead of a String. Then this line will throw error

TypeError: expected str, bytes or os.PathLike object, not NoneType

which is not caught by FileNotFoundError below.

Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest opening a new issue to track these problems instead of continuing an already closed PR thread.

"endpoint": endpoint
or Configuration().EXPORTER_OTLP_SPAN_ENDPOINT,
"insecure": insecure,
"credentials": credentials,
Copy link
Contributor

Choose a reason for hiding this comment

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

If I set OTLPExporter(insecure=True) this works fine, but if I leave that out, I get a message

File "/Users/enowell/git/opentelemetry-python/exporter/opentelemetry-exporter-otlp/src/opentelemetry/exporter/otlp/exporter.py", line 178, in __init__
    self._client = self._stub(secure_channel(endpoint, credentials))
  File "/Users/enowell/Library/Python/3.8/lib/python/site-packages/grpc/__init__.py", line 1943, in secure_channel
    if credentials._credentials is _insecure_channel_credentials:
AttributeError: 'NoneType' object has no attribute '_credentials'

which makes sense because insecure=None and credentials=None. I think a follow up would be to throw an Error Message if insecure=None (or False) && credentials=None saying "Error: Either provide credentials or label as insecure."

Choose a reason for hiding this comment

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

@NathanielRN Thanks for reporting this. I also ran into this issue. Causes a failure in the example code.

@jan25
Copy link
Contributor Author

jan25 commented Oct 31, 2020

@NathanielRN thanks for the review! I'll work on fixing them and open a PR. I couldn't think of way to add such case in unittests, will try again.

srikanthccv pushed a commit to srikanthccv/opentelemetry-python that referenced this pull request Nov 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release:required-for-ga To be resolved before GA release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support env variable configuration for OTLP exporter
6 participants