feat: Add basic support for schema_url #1237
Open
+197
−68
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ahayworth
requested review from
arielvalentin,
dazuma,
ericmustin,
fbogsany,
mwear,
plantfansam and
robertlaurin
as code owners
Closed
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?*
ahayworth
force-pushed
the
ahayworth/schema-url
branch
from
57ed60e to
444501c
Compare
This comment was marked as resolved.
This comment was marked as resolved.
ahayworth
added
spec-compliance
fbogsany
reviewed
May 20, 2022
fbogsany
reviewed
May 20, 2022
fbogsany
reviewed
May 20, 2022
fbogsany
reviewed
May 20, 2022
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.
fbogsany
reviewed
May 24, 2022
fbogsany
reviewed
May 24, 2022
fbogsany
reviewed
May 24, 2022
| self.class.send(:new, attributes.merge(other.attributes).freeze, new_schema_url.freeze) | ||
| end | ||
|
|
||
| attr_reader :schema_url |
Contributor
There was a problem hiding this comment.
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
Contributor
Author
There was a problem hiding this comment.
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.
4 tasks
2 tasks
Contributor
|
👋 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 |
kaylareopelle
requested review from
kaylareopelle and
robbkidd
as code owners
github-project-automation
bot
moved this from Done
to In progress
in Ruby - Spec Compliance
Contributor
|
As we get closer to moving to stable semantic conventions in our HTTP instrumentation libraries, we could use support for |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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?