-
Notifications
You must be signed in to change notification settings - Fork 247
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 support for automated resource detection #263
Add support for automated resource detection #263
Conversation
|
||
resource_labels = {} | ||
resource_labels[CLOUD_RESOURCE[:provider]] = 'gcp' | ||
resource_labels[CLOUD_RESOURCE[:account_id]] = gcp_env.project_id || '' |
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 deserves a second set of eyes I'm not totally sure this is the correct value being set here, I was following the work done on the JS library https://github.com/open-telemetry/opentelemetry-js/pull/899/files#diff-2ae8d53e5ba7424080459738d6bc210bR45
From the spec
The cloud account id used to identify different entities.
But this is using project-id
from https://cloud.google.com/compute/docs/storing-retrieving-metadata#default
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.
project-id
seems wrong, but the metadata doesn't supply the account ID. I would suggest leaving this as-is and opening an issue in the spec repo asking for clarification.
dba4e58
to
af12336
Compare
af12336
to
9f68043
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.
This is off to a good start @robertlaurin. I made a first pass of comments and realize it's a draft.
sdk/lib/opentelemetry/sdk/resources/detectors/google_cloud_platform.rb
Outdated
Show resolved
Hide resolved
sdk/lib/opentelemetry/sdk/resources/detectors/google_cloud_platform.rb
Outdated
Show resolved
Hide resolved
078eb83
to
ec04cac
Compare
resource-detectors/lib/opentelemetry/resource/detectors/google_cloud_platform.rb
Show resolved
Hide resolved
resource-detectors/lib/opentelemetry/resource/detectors/auto_detector.rb
Outdated
Show resolved
Hide resolved
Related: open-telemetry/oteps#111 |
@robertlaurin can you please resolve any out-of-date comments above and rebase? This LGTM and I’ve approved it, but it wasn’t clear to me if you still had unresolved questions. Selfishly, I need this merged for my work on the OTLP exporter 😄 . |
@fbogsany I've resolved every I believe to be resolved 😄 There's only two points left that I'd like to leave to some one else to resolve. I don't think this one is a problem but I raised it just to get a second set of eyes on it. The long message thread here, once again I think this one is good but I'd like to leave it to either you or @mwear to sign off on it. |
@@ -56,7 +54,8 @@ def internal_create_span(result, name, kind, trace_id, span_id, parent_span_id, | |||
attributes = attributes&.merge(result.attributes) || result.attributes | |||
active_trace_config = OpenTelemetry.tracer_provider.active_trace_config | |||
active_span_processor = OpenTelemetry.tracer_provider.active_span_processor | |||
Span.new(context, name, kind, parent_span_id, active_trace_config, active_span_processor, attributes, links, start_timestamp || Time.now, @resource, @instrumentation_library) | |||
resource = OpenTelemetry.tracer_provider.resource | |||
Span.new(context, name, kind, parent_span_id, active_trace_config, active_span_processor, attributes, links, start_timestamp || Time.now, resource, @instrumentation_library) |
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 think this is incorrect. Or at least unnecessary. We should remove the resource
parameter -- for now, can you just pass nil
?
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.
What the issue here exactly? A span should have a reference to a resource.
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.
A span needs a reference to an InstrumentationLibrary
. The Resource
is global (a singleton). I don’t see the need to attach the same Resource reference to every span when it is always available from OpenTelemetry.tracer_provider.resource
.
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.
Unfortunately, that's a current design flaw of the Ruby library. Technically, multiple TracerProviders should be able to exist in a single process. They should be able to have different resources, but share the same export pipeline. So, the resource is not exactly a global singleton. It's my understanding that in some languages, it's common to have multiple applications per process. .NET is one that I'm aware of. This was discussed somewhat in open-telemetry/opentelemetry-specification#508 and I am supposed to be writing some clarifying language for the spec about this topic.
For the Ruby SDK. I think it's fine to have a single TracerProvider configured by default, as long as we allow additional ones to be instantiated. We'll need to make sure that references go through TracerProvider instances, rather than the classes. There are few places we were need to clean up static references in this regard.
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'll try to get some more clarification about the multiple TracerProvider situation at the spec SIG tomorrow. I've heard that it's a use case, but given that you can only have one global TracerProvider at the API level, I'm not really sure how this works in practice, or if it's necessarily a use case that needs to be preserved.
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 the relevant portion of the spec: https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/trace/api.md#tracerprovider
Normally, the TracerProvider is expected to be accessed from a central place. Thus, the API SHOULD provide a way to set/register and access a global default TracerProvider.
Notwithstanding any global TracerProvider, some applications may want to or have to use multiple TracerProvider instances, e.g. to have different configuration (like SpanProcessors) for each (and consequently for the Tracers obtained from them), or because its easier with dependency injection frameworks. Thus, implementations of TracerProvider SHOULD allow creating an arbitrary number of TracerProvider instances.
Noting that the spec lists this as a SHOULD, not a MUST.
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.
If I'm following correctly, for the sdk/lib/opentelemetry/sdk/trace/tracer.rb
code could we not "just" pass in a reference to the tracer_provider
that created it? The tracer_provider
is accessed in multiple places within the tracer.rb
class already, why not pass it in as part of the initializer for the tracer?
That would solve the issue of supporting multiple tracer_provider
s for this particular code path, no?
It would also remove some of the implicit coupling that the tracer has on the tracer_provider
existing.
https://github.com/robertlaurin/opentelemetry-ruby/blob/cf02a49c38e230f805dcfd9bbb39b8b9a8caabfb/sdk/lib/opentelemetry/sdk/trace/tracer.rb#L27
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.
You're following correctly @robertlaurin. I think the core issue is that because of the multiple TracerProvider use case, each span needs a reference to a resource. Ultimately, this is because TracerProviders can be initialized with different resources, but share the same export pipeline. As you point out, we can support this use case pretty easily passing a TracerProvider to a Tracer during initialization and maintaining a reference to it.
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'm not sure how much of this needs to be addressed in this PR. It would be fine to create issues for additional work that isn't directly related.
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 agree that I don't want to grow this PR into additional work that isn't directly related.
I think this change here a43ce3a leaves the code in a good places to be improved on.
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 pretty good to me, just a few comments. I'd like to get a little more clarification as to the questions around the instrumentation library.
resource-detectors/test/opentelemetry/detectors/google_cloud_platform_test.rb
Outdated
Show resolved
Hide resolved
resource-detectors/test/opentelemetry/detectors/google_cloud_platform_test.rb
Outdated
Show resolved
Hide resolved
@@ -56,7 +54,8 @@ def internal_create_span(result, name, kind, trace_id, span_id, parent_span_id, | |||
attributes = attributes&.merge(result.attributes) || result.attributes | |||
active_trace_config = OpenTelemetry.tracer_provider.active_trace_config | |||
active_span_processor = OpenTelemetry.tracer_provider.active_span_processor | |||
Span.new(context, name, kind, parent_span_id, active_trace_config, active_span_processor, attributes, links, start_timestamp || Time.now, @resource, @instrumentation_library) | |||
resource = OpenTelemetry.tracer_provider.resource | |||
Span.new(context, name, kind, parent_span_id, active_trace_config, active_span_processor, attributes, links, start_timestamp || Time.now, resource, @instrumentation_library) |
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.
What the issue here exactly? A span should have a reference to a resource.
LGTM. Needs a rebase, but otherwise 👍 |
This adds support for populating a resource with with Telemetry data as well as Google Cloud Platform environment metata data. For open-telemetry#230
To avoid introducing third party dependencies to the SDK for optional functionality resource detection has been extracted into its own gem.
The specification requires a Telemetry::SDK resource, this adds a method for create it directly from the resource class. This also initializes it by default on the configurator. The resource setter on the configurator class will merge the default resource with the one provided.
Add resource documentation for configurator. Update comments for resource constants. Simplify resource merging in auto detector.
The tracer carries a reference to the tracer_provider that initialized it to begin supporting multiple tracer providers.
a43ce3a
to
83780db
Compare
This adds support for populating a resource with with Telemetry data
as well as Google Cloud Platform environment metata data.
For #230
Example usage