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.
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
Telemetry Schema and Resource #161
base: main
Are you sure you want to change the base?
Telemetry Schema and Resource #161
Changes from all commits
51c66b1
d9a3368
ea3d30d
b0cf166
a83f054
520369f
71155a1
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 we should provide only the "go up" version, and enforce everyone to use the most recent (latest) schema. The exporter in your case should be able to adopt (maybe) the rules to work on the Resource schema (if newer) vs downgrading the 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.
We can start by implementing only go-up, but as I stated, there's an issue with mismatch between various sourced components. If we're not somewhat compatible in a forwards way, we could cause issues across the ecosystem.
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.
We should be comfortable with both up and down directions. The schema files unambiguously defines whether it is safe to go in a particular direction.
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.
Is this a long term issue or just a short term need, I had the impression that we should always do backwards compatible changes. So we can always go up, sometimes we may not be able to go down.
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 an issue of how users consume exporters + resource detecters in the SDK. Both are pluggable components and we can expect resource detection to advance ahead of exporters, so the forward-compatible behavior is needed in that kind of distribution environment.
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.
Should we say that similar changes will be applied to other places where schemaurl is used, like spans/metrics/logs?
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.
Eventually, that would be the goal.
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! This is an expected capability of schemas, so no surprises here.
I believe this should also be able to return an error since some conversions are not possible (e.g. between different schema families or if the versions are incompatible).
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.
Since Resource resolution happens at application startup, I'm nervous about having SDKs have to go and fetch multiple schemas from the public internet as a part of the startup process, in order to perform the conversion. There are failure modes here, and general startup-time issues to be considered (especially, for example, in mobile or FAAS implementations).
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.
That's a good point. The SDKs can have the schema file that they correspond to embedded with the source code so that there is no need to fetch anything for detectors that are part of the SDK. If each SDK includes the latest Otel schema file then any Otel schema conversions can happen without network access.
Custom, non-Otel schema conversions will still require a network fetching. Perhaps we also provide an alternate
convertToSchema
function which accepts a schema file instead of 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.
I don't think that an alternate conversion method should be required to work from a file, but an alternate URL->bytes function could be provided. I'd imagine possibly a three tier lookup: 1) embedded in the application by the SDK, 2) on disk in a well-known or specified location 3) over the network through traditional URL resolution.
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.
Per this thread conversation, it may be better to start with a solution that "file"/"bytes" for the schema are provided, and delegate to the user of the API to ensure that is available somewhere to grab. Some users may embed the files in their application, some may embed the schema bytes, etc.
@jkwatson had a very good point, and probably we should postpone the API that fetches from the internet.
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 with the notion of providing the local schema instead of fetching from the URL. We may go a step further and define
Schema
as an opaque object that is loadable from file or from a URL. This opens up the possibility of performance improvements (parse and compile the schema once such that the subsequent conversion calls are fast). See e.g. prototype here that has "parse/compile" and "convert" as separate operations.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.
Not necessary related question: Is this the expected value for the proto fields or just the string number "1.1.0"?
/cc @tigrannajaryan
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.
Same question, is this something that we need to worry? Should we assume that converting to the "newest" is always possible?
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.
Is this section needed because currently the schema url does not respect semver for the version number? I though that was implicit since the schema url version number corresponds to the specs version which follows semver, probably my assumption is wrong.
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.
The Schema URL could be for a different specification entirely. This OTEP proposed a level of compatibility we could not assume before.
This is needed so we CAN assume semver for the default otel schema.
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 a little confused on this. If you had a schema rule like the one above:
since there is a transform between versions, would the two schemas be compatible and have the same major version?
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's not clear to me what the answer to this is, which is why I opened: #162
IF the answer to the quesiton is "yes they're compatible", then I think this OTEP resolves some major pain that kind of change would cause.
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 seems that "older resource" may be used in two senses here. One being the target of a merge operation and the other being the resource with the lesser schema version. Utilizing a different term for one or both of those senses may help avoid confusion.
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.
"major version bump" of what?
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.
the schema version
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.
So this would be a "poor mans" conversion function, which just figures out that the conversion is noop and can just return the original and fail otherwise, right? I believe this should cover a very large number of real-world use cases of existing resource detectors.
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.
Exactly.
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.
Would this check be limited to attributes actually present on the resources being evaluated, or would the presence of any transformation, even for non-present attributes, prevent merging/migration?
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 defined by schema OTEP. Conversion between families is incompatible. Ambiguous "renames" are incompatible.
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 it is safe. Any reason to believe it may not?
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 we should make them independent part of SDKs, consumable separately if possible by code that does not care about the rest of Otel but needs to deal with schemas. It will be very nice though to make this an official part of Otel libaries.
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 need this because you want to be able to also convert Spans/Metrics/Logs that also have schemaURL.