-
Notifications
You must be signed in to change notification settings - Fork 624
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
Rename otcollector to opencensus #695
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking pretty good, thanks for the PR! I would like to see an integration test against an OpenCensus collector before approving this.
ext/opentelemetry-ext-opencensus/src/opentelemetry/ext/opencensus/__init__.py
Outdated
Show resolved
Hide resolved
@@ -18,7 +18,7 @@ | |||
.. code:: python | |||
|
|||
from opentelemetry import trace | |||
from opentelemetry.ext.otcollector.trace_exporter import CollectorSpanExporter | |||
from opentelemetry.ext.opencensus.trace_exporter import CollectorSpanExporter |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should the name of the exporters be updated to reflect this is specifically for the OpenCensus Collector as opposed to the OpenTelemetry collector? I don't know if it will create confusion to have multiple CollectorSpanExporter
or if the package naming is differentiating enough.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I fixed some naming in the last commit, please let me know if it is right now 👍
611521a
to
02412de
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, is great this one come with docker tests as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great, thanks for adding the integration tests. Just one question I'm waiting for a response on, otherwise I'll approve and merge.
...ext-opencensusexporter/src/opentelemetry/ext/opencensusexporter/metrics_exporter/__init__.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
…sus/__init__.py Co-authored-by: alrex <[email protected]>
This is done to keep the test code while the OpenCensus issue reported (census-instrumentation/opencensus-service#641) gets fixed.
Fixes #541