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

Jump to bottom

Add OTLP span exporter #787

Merged
merged 24 commits into from
Jun 10, 2020
Merged

Add OTLP span exporter #787

merged 24 commits into from
Jun 10, 2020

Conversation

ocelotl
Copy link
Contributor

@ocelotl ocelotl commented Jun 7, 2020

Fixes #786

@ocelotl ocelotl added the ext label Jun 7, 2020
@ocelotl ocelotl requested a review from a team June 7, 2020 00:15
@ocelotl ocelotl self-assigned this Jun 7, 2020
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Jun 7, 2020 •
edited
Loading

CLA Check
The committers are authorized under a signed CLA.

codeboten
codeboten suggested changes Jun 8, 2020
View reviewed changes
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.

At first pass this is looking pretty good. I've made some suggestions in the PR, but I would like to see the following before approving:

  • _translate_spans method broken up, makes it easier to review and understand (as well as test)
  • docs entry for the exporter
  • changelogs entries
  • version numbers updated

ext/opentelemetry-ext-otlpexporter/README.rst Outdated Show resolved Hide resolved
ext/opentelemetry-ext-otlpexporter/README.rst Outdated Show resolved Hide resolved
ext/opentelemetry-ext-otlpexporter/README.rst Outdated Show resolved Hide resolved
ext/opentelemetry-ext-otlpexporter/README.rst Outdated Show resolved Hide resolved
ext/opentelemetry-ext-otlpexporter/setup.cfg Outdated Show resolved Hide resolved
...opentelemetry-ext-otlpexporter/src/opentelemetry/ext/otlpexporter/trace_exporter/__init__.py Outdated Show resolved Hide resolved
...opentelemetry-ext-otlpexporter/src/opentelemetry/ext/otlpexporter/trace_exporter/__init__.py Outdated Show resolved Hide resolved
...opentelemetry-ext-otlpexporter/src/opentelemetry/ext/otlpexporter/trace_exporter/__init__.py Outdated Show resolved Hide resolved
...opentelemetry-ext-otlpexporter/src/opentelemetry/ext/otlpexporter/trace_exporter/__init__.py Outdated Show resolved Hide resolved
ext/opentelemetry-ext-otlpexporter/CHANGELOG.md Outdated Show resolved Hide resolved
@codeboten codeboten added this to the Beta v0.9 milestone Jun 8, 2020
@ocelotl ocelotl changed the title Add OTLP exporter Add OTLP span exporter Jun 8, 2020
@ocelotl ocelotl force-pushed the issue_785 branch 5 times, most recently from 49b310f to 67b96d8 Compare June 9, 2020 16:22
lzchen
lzchen reviewed Jun 10, 2020
View reviewed changes
ext/opentelemetry-ext-otlp/src/opentelemetry/ext/otlp/trace_exporter/__init__.py
+ retry_info.retry_delay.nanos / 1.0e9
)

sleep(delay)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm wondering how this would work with the batchspanprocessor's worker thread. Wouldn't sleep potentially block this thread from running for 15 minutes, so no other spans can be exported?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, that would happen, but during the time the exporter blocks the batch span processor would have its queue filled up and will continue to export them once the block is over

@lzchen
Copy link
Contributor

lzchen commented Jun 10, 2020

Also, does this PR coming out mean that the OpenTelemetry receiver is ready in OT collector?

@codeboten
Copy link
Contributor

Also, does this PR coming out mean that the OpenTelemetry receiver is ready in OT collector?

It is! A few other sigs already implement native OTLP

codeboten
codeboten approved these changes Jun 10, 2020
View reviewed changes
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.

This is looking great. Thanks for doing this work

ext/opentelemetry-ext-otlp/setup.cfg Outdated Show resolved Hide resolved
ext/opentelemetry-ext-otlp/setup.cfg Show resolved Hide resolved
ext/opentelemetry-ext-otlp/src/opentelemetry/ext/otlp/trace_exporter/__init__.py
return ExportTraceServiceRequest(resource_spans=resource_spans)

def export(self, spans: Sequence[SDKSpan]) -> SpanExportResult:
# expo returns a generator that yields delay values which grow
Copy link
Contributor

Choose a reason for hiding this comment

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

looks like my question got lost, but any advantages of using expo instead of backoff's @backoff.on_exception decorator here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here is the question 😉

lzchen
lzchen approved these changes Jun 10, 2020
View reviewed changes
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.

LGTM!

@ocelotl ocelotl added the PR:please merge This PR is ready to be merged by a Maintainer (has enough valid approvals, successful build, etc.) label Jun 10, 2020
ocelotl and others added 21 commits June 10, 2020 16:43
@ocelotl @codeboten
Update ext/opentelemetry-ext-otlpexporter/README.rst
b062596 
Co-authored-by: alrex <[email protected]>
@ocelotl @codeboten
Update ext/opentelemetry-ext-otlpexporter/README.rst
f87eb1c 
Co-authored-by: alrex <[email protected]>
@ocelotl
Add link to OTLP definition
ac6bd3b
@ocelotl
Fix versioning
a816df4
@ocelotl
Rename to otlp
87efd36
@ocelotl
Update version
67a30a3
@ocelotl
Fix time value
dc05b27
@ocelotl
Split method
98c428d
@ocelotl
Fix lint
68f3324
@ocelotl
Add doc
b3d44e2
@ocelotl
Remove 3.4
f4da6ef
@ocelotl
Wow, more lint fixing
7e46d27
@ocelotl
Remove pypy
adc82f4
@ocelotl
Use ordered dictionaries
95b5ee0
@ocelotl @lzchen
Update ext/opentelemetry-ext-otlp/src/opentelemetry/ext/otlp/trace_ex…
114321a 
…porter/__init__.py

Co-authored-by: Leighton Chen <[email protected]>
@ocelotl
Move method outside of class
2c5a479
@ocelotl
Add docs
b0cd0d7
@ocelotl
Fixed dependencies
1ad583f
@ocelotl
Add proto to build.sh
14817d5
@ocelotl
Add logging
623667c
@ocelotl
Fix lint
cfce577
@codeboten codeboten merged commit 376c43e into open-telemetry:master Jun 10, 2020
codeboten pushed a commit to codeboten/opentelemetry-python that referenced this pull request Jun 11, 2020
@ocelotl @lzchen
ext/otlp: Add OTLP span exporter (open-telemetry#787)
a300c16 
Co-authored-by: Leighton Chen <[email protected]>
Co-authored-by: alrex <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lzchen lzchen lzchen approved these changes

@codeboten codeboten codeboten approved these changes

@toumorokoshi toumorokoshi Awaiting requested review from toumorokoshi

@c24t c24t Awaiting requested review from c24t

Assignees

@ocelotl ocelotl

Labels
PR:please merge This PR is ready to be merged by a Maintainer (has enough valid approvals, successful build, etc.)
Projects
None yet
Milestone
Beta v0.9
Development

Successfully merging this pull request may close these issues.

Add OTLP span exporter
3 participants
@ocelotl @lzchen @codeboten