-
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
Resources prototype #853
Resources prototype #853
Conversation
ext/opentelemetry-exporter-cloud-trace/src/opentelemetry/tools/resource_detector.py
Outdated
Show resolved
Hide resolved
ext/opentelemetry-exporter-cloud-trace/src/opentelemetry/tools/resource_detector.py
Outdated
Show resolved
Hide resolved
ext/opentelemetry-exporter-cloud-trace/src/opentelemetry/tools/resource_detector.py
Outdated
Show resolved
Hide resolved
ext/opentelemetry-exporter-cloud-trace/src/opentelemetry/tools/resource_detector.py
Outdated
Show resolved
Hide resolved
ext/opentelemetry-exporter-cloud-trace/src/opentelemetry/tools/resource_detector.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.
Mostly superficial comments, the direction LGTM.
...ntelemetry-exporter-cloud-monitoring/src/opentelemetry/exporter/cloud_monitoring/__init__.py
Outdated
Show resolved
Hide resolved
ext/opentelemetry-exporter-cloud-monitoring/tests/test_cloud_monitoring.py
Outdated
Show resolved
Hide resolved
ext/opentelemetry-exporter-cloud-trace/src/opentelemetry/tools/resource_detector.py
Outdated
Show resolved
Hide resolved
ext/opentelemetry-exporter-cloud-trace/src/opentelemetry/tools/resource_detector.py
Outdated
Show resolved
Hide resolved
ext/opentelemetry-exporter-cloud-trace/src/opentelemetry/exporter/cloud_trace/__init__.py
Show resolved
Hide resolved
ext/opentelemetry-exporter-cloud-trace/src/opentelemetry/tools/resource_detector.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.
Changes since the last review LGTM, thanks for updating!
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.
I do think that we should put the GCP resource detection code in a separate package to the trace/metric exporters, as users may want to use the GCP resource detector with a diff BE/exporter, but we can review that later
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.
Nice! What's the plan for actually registering the detectors the user wants?
ext/opentelemetry-exporter-cloud-trace/src/opentelemetry/tools/resource_detector.py
Show resolved
Hide resolved
ext/opentelemetry-exporter-cloud-trace/src/opentelemetry/tools/resource_detector.py
Outdated
Show resolved
Hide resolved
ext/opentelemetry-exporter-cloud-trace/src/opentelemetry/tools/resource_detector.py
Outdated
Show resolved
Hide resolved
ext/opentelemetry-exporter-cloud-trace/src/opentelemetry/tools/resource_detector.py
Outdated
Show resolved
Hide resolved
ext/opentelemetry-exporter-cloud-trace/tests/test_gcp_resource_detector.py
Outdated
Show resolved
Hide resolved
@aabmass for your comment, I plan on adding documentation + examples for both the new propagator and resource detector once we migrate to the new repo. Issue is here GoogleCloudPlatform/opentelemetry-operations-python#13. |
ext/opentelemetry-exporter-cloud-trace/tests/test_gcp_resource_detector.py
Outdated
Show resolved
Hide resolved
ext/opentelemetry-exporter-cloud-trace/src/opentelemetry/tools/resource_detector.py
Outdated
Show resolved
Hide resolved
...ntelemetry-exporter-cloud-monitoring/src/opentelemetry/exporter/cloud_monitoring/__init__.py
Show resolved
Hide resolved
ext/opentelemetry-exporter-cloud-trace/src/opentelemetry/exporter/cloud_trace/__init__.py
Show resolved
Hide resolved
Currently, resource detection is being multi threaded. I'm not too aware of the ramifications of that (stuff like needing to add "suppress_instrumentation" to the context) and would love to get some feedback in that direction :) |
ext/opentelemetry-exporter-cloud-trace/src/opentelemetry/tools/resource_detector.py
Show resolved
Hide resolved
I'm not sure what you mean by |
95f4d66
to
b98c061
Compare
Co-authored-by: Connor Adams <[email protected]>
Adds Resource Detection for Google tools like GCE and for environment variable resource detection OTEL_RESOURCE. Follows open-telemetry/oteps#111 (Note at the time of writing this OTEP hasn't been updated after our offline discussion so there's currently some discrepancy).
Example:
Tests will be added in the next commit. Doing some manual testing with the code above, it correctly scrapes information when run on a GCE instance and sends it to Monitoring properly.
Addresses issue GoogleCloudPlatform/opentelemetry-operations-python#8