Add schema_url support for tracer, resource, resource detectors #979
Conversation
… todo add to exporter, resource, resource detector, and account for merging and export behavior
…o is updated, update resource and resource attributes
ericmustin
requested review from
ahayworth,
arielvalentin,
dazuma,
fbogsany,
mwear and
robertlaurin
as code owners
|
#982 should unblock
|
| # will use the applicaiton wide (resource) schema_url. If an application wide | ||
| # (resource) schema_url cannot be found, it defaults to remaining unset and | ||
| # is set by the OTLP exporter using the schema of the exporter. | ||
| # TODO: clarify this^, is this correct? where does app wide schema url get used? |
There was a problem hiding this comment.
I think we can just replace this with an attr_writer :instrumentation_schema_url. We don't infer anything. It defaults to nil, and if it is nil, backends will use the Resource schema_url instead.
There was a problem hiding this comment.
this sounds correct to me, but when i tried to move this into attr_writer :instrumentation_schema_url couldn't seem to get it to work well the tests. I went down a rabbit hole of trying to understand why and couldn't come up with a clear answer, will try to look at this with fresh eyes. I think our pattern of instantiating an instrumentation instance doesn't play nicely with this approach.
other than this everything else should be resolved afaik
There was a problem hiding this comment.
i'm kinda stumped here. Changing to an attr_writer or attr_accessor causes a slight pattern change in the way an instrumentation author defines an instrumentation_schema_url (instrumentation_schema_url = <schema_url>) vs how they define instrumentation_version (instrumentation_version(<version>)) or instrumentation_name. (instrumentation_name(<name>) in their instrumentation Class. Updating test classes to account for this, it still seems to be problematic when we create a new instance. But defining the method instead of using attr_ makes this problem go away. Maybe attr_ doesn't define the getters and setters at the appropriate time, whereas defining it explicitly does? 🤷
| @@ -42,15 +45,23 @@ def telemetry_sdk | |||
| } | |||
|
|
|||
| resource_pairs = ENV['OTEL_RESOURCE_ATTRIBUTES'] | |||
| return create(resource_attributes) unless resource_pairs.is_a?(String) | |||
|
|
|||
| # TODO: Since we're using a specific semcon version, my interpretation of schema_url is that we should define | |||
There was a problem hiding this comment.
I don't think we should provide a schema_url for any resources created by the SDK. If we provide a schema_url and the application wants to provides its own (different) schema_url, then they can't use the resources provided by the SDK because they won't merge.
There was a problem hiding this comment.
Yea i think i misread the specification, my understanding of the specification was that they can merge, it is only the schema_url that would be undefined https://github.com/open-telemetry/opentelemetry-specification/blob/ef6e2f41a905a8c9b89d9e0895d9648ee7645f65/specification/resource/sdk.md#merge
But reviewing the python implementation, the merge itself would be an error (and return the old resource): https://github.com/open-telemetry/opentelemetry-python/blob/3cdb54f6085ce7170aea05e2c99c480a6e919f34/opentelemetry-sdk/src/opentelemetry/sdk/resources/__init__.py#L195-L229
So, can remove this (and i think i have to update the merge behavior too so that the implementation is correct), although it does seem like we technically are using a specific schema within these sdk created resources, so it's a bit odd to me that we can't represent that easily.
…esources to match spec
|
@ericmustin I totally didn't realize this was already open before I started working on it in #1237 🤦 If it's okay with you, I'll close this PR out and we can keep going with #1237, since that's a little more up to date at this point? I copied over some of the resource_detector / instrumentation-base stuff from this PR, since I had neglected it previously... |
|
@ahayworth no worries, closing this |
Summary
Addresses #969
Still a few loose ends and questions here imo, but I think this is the majority of the work.
TODOs:
blocked until we upgrade from v0.4.0 to v0.9.0 otlp protounblocked, upgraded to v0.11.0)Notes
This PR adds schema url to the tracer, instrumentation library, and resource + resource detector. I am a little bit unsure how to handle schema_url in the
service_name=andservice_version=configurator methods, since it seems like this could have some unintended consequences? In general the resource implementation is a bit unclear to me. I'm also using a lot of interpolated strings inline that probably ought to be abstacted out into some nice clean helper but wasn't sure what the best practice was there, open to feedback.Also dummied out the otlp exporter usage ofthis is updated now, disregard.schema_urlsince the proto we're using is v0.4.0 and thus doesn't have support (support only exists in v0.9.0)I'm still a bit unclear on the spec so it's possible i've messed something up here, feedback appreciated