-
Notifications
You must be signed in to change notification settings - Fork 232
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
feat: Add basic support for schema_url #1237
Conversation
This adds basic schema URL support in the API + SDK, specifically in and around Resources, Tracers, Tracer Provider, and the OTLP exporter. Some notes, in no particular order: - I did not add schema URLs to any of our instrumentation, because that was a lot of work. - The _API_ ProxyTracer tests around "asking for a tracer multiple times returns the same thing" don't actually work, because we don't do anything with the name+version(+schema url, after this change) that you pass in. I don't think that matters, but I'm not really sure. - I didn't add the schema information to the jaeger/zipkin exporters, after surveying what Go and Python were doing. I don't see an easy place to add it in, either. 1. The behavior of merging two resources with different, non-empty schema URLs is implementation dependent. I chose to _drop_ the schema URL in that case and continue merging resources as before. *Is this what we want?* Other SDKs do things differently (Go returns an error, Python logs an error and returns the old resource, etc). 2. I did *not* add support to the Configurator for setting the schema_url on the default Resource that gets initialized. That seemed like the kind of thing folks would do incorrectly, but you can still do such a thing by creating and assigning an entire Resource if you wish. *Is this what we want?*
57ed60e
to
444501c
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Co-authored-by: Francis Bogsanyi <francis.bogsanyi@shopify.com>
Per Francis, this is actually by design...
We're going to need to add other things here eventually...
The spec says "empty" - which you could say is possibly an empty string (which was what I did initially). But given that an `InstrumentationLibrary` in this implementation is just a struct, an ILS initialized without a schema_url will actually get one that is `nil`, not `''` Just better to be consistent. We treat empty-string schema_urls as if they were opaque; don't do that unless you really want it.
end | ||
|
||
attr_reader :schema_url |
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.
It is interesting to note that the spec doesn't say anything about a getter for schema_url
. Obviously, we need one, since exporters need to be able to read it. We should open a PR against the spec to add this after https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/resource/sdk.md#retrieve-attributes
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.
Mmm - good point.
Co-authored-by: Francis Bogsanyi <francis.bogsanyi@shopify.com>
The ugly thing is it requires curly braces around your attributes, all the time.
Any instrumentation can now set an `instrumentation_schema_url` if they so choose. Resource detectors may also do so. Notably, for the GCP resource detector, we explicitly pass a `nil` schema_url. That's the default, of course, but I made it explicit since we haven't actually audited whether or not we conform to any schema.
👋 This pull request has been marked as stale because it has been open with no activity. You can: comment on the issue or remove the stale label to hold stale off for a while, add the |
This adds basic schema URL support in the API + SDK, specifically in and
around Resources, Tracers, Tracer Provider, and the OTLP exporter.
Some notes, in no particular order:
was a lot of work.
returns the same thing" don't actually work, because we don't do
anything with the name+version(+schema url, after this change) that
you pass in. I don't think that matters, but I'm not really sure.
after surveying what Go and Python were doing. I don't see an easy
place to add it in, either.
Questions:
schema URLs is implementation dependent. I chose to drop the schema
URL in that case and continue merging resources as before. Is this
what we want? Other SDKs do things differently (Go returns an error,
Python logs an error and returns the old resource, etc).
schema_url on the default Resource that gets initialized. That seemed
like the kind of thing folks would do incorrectly, but you can still
do such a thing by creating and assigning an entire Resource if you
wish. Is this what we want?