Create minimal Entity SDK specification#5201
Conversation
Co-authored-by: Jack Berg <[email protected]>
|
I made a quick PoC for Go, and it seems straightforward (some small API design aspects to work out): open-telemetry/opentelemetry-go#8610. How does this interact with #4665? IIRC, that required entities to be part of the API. Would we have two different entity definitions? Or do we need to define entities outside of the SDK for this spec change as well? |
@dashpole We already have definitions "shadowed" between API and SDK. I don't think we've ever promoted an SDK concept into API before - but that's how I envision that transition. We would define an interface for the API similar to |
Co-authored-by: Robert Pająk <[email protected]>
Co-authored-by: Dmitry Anoshin <[email protected]>
pellared
left a comment
There was a problem hiding this comment.
Specification how entities affect existing operations (Retrieve attributes) is needed
This comment was marked as outdated.
This comment was marked as outdated.
| **Status**: [Experimental](../document-status.md) | ||
|
|
||
| When a Resource contains entities, the merge operation MUST follow the | ||
| [resource data model's merge algorithm](./data-model.md#merging-resources). |
There was a problem hiding this comment.
This adds another MUST without replacing the preceding attribute and Schema URL rules. For example, merging resources whose entities use different Schema URLs must produce a blank resource Schema URL under the entities data-model algorithm
opentelemetry-specification/specification/resource/data-model.md
Lines 81 to 92 in 8db7da2
but these lines
opentelemetry-specification/specification/resource/sdk.md
Lines 94 to 102 in 8db7da2
says that when this would occur for "regular" attributes then this would be also a merge error.
Can we clarify it this should still return a merge error?
For reference:
There was a problem hiding this comment.
I noticed that this is implemented in open-telemetry/opentelemetry-go#8610 as a merge error
There was a problem hiding this comment.
Great callout - I'll update this shortly.
Briefly - the new algorithm is designs so it would do the same thing the previous algorithm did in absence of entities. One entities exist, the schema_url on Resource is for less useufl, and the one in the Entity is the important one. Preserving Resource's schema_url is best effort, and the merge algorithm will do this.
So this is meant to be a non-breaking change in that, in practice, you couldn't rely on schema_url for resource in most cases.
There was a problem hiding this comment.
Updated again - making it more clear the actual goal of schema url in the note. This should be pretty explicit that Resource.schema_url is not generally useful and actively problematic and any algorithmic features around preserving it are best effort not something we should rely on.
There was a problem hiding this comment.
Answered this also in #5201 (comment)
With the proposed entity-aware algorithm, the same flattened attributes could instead clear Resource.schema_url solely because those attributes are represented by an Entity with an empty Schema URL. That gives Resource.schema_url different merge semantics depending on whether Entities are present. This is particularly surprising when the PR also describes that field as being retained for backwards compatibility and not used by entity-aware systems.
There was a problem hiding this comment.
Giving the discussions I think it would be a lot more pragmatic to try addressing my concern in a separate PR/issue. Probably I can work on it mid-August). Feel free to resolve.
There was a problem hiding this comment.
I think we do have an answer for you - and I'm happy to work on eluci8dating that in spec / docs. We'll leave this bug (#5201) open/unresolved. I'd like to merge this PR - but we'll make sure our next task is resolving your concerns - particularly making sure the link you sent is not broken as entities are layered in.
Co-authored-by: Robert Pająk <[email protected]>
| The resulting `SchemaURL` MUST match the behavior defined in the merge | ||
| algorithm, which is compatible with previous resource behavior - and updated | ||
| to account for `SchemaURL` being tracked in Entities. |
There was a problem hiding this comment.
I am not sure how it helps. I feel that the "Merging Entities into a Resource" algorithms is conflicting with the existing "Merging" algorithm and that we need to fix the "Merging Entities into a Resource" section.
opentelemetry-specification/specification/resource/data-model.md
Lines 81 to 83 in 8db7da2
First example
What should we do when all entities within E have the same (non-empty) schema_url and the resource (non-empty) schema_url is different? The current version says that it should replace the schema_url which can be different. In my opinion, this behavior is in conflict with
opentelemetry-specification/specification/resource/sdk.md
Lines 100 to 102 in 8db7da2
I do not think we should specify what to do with the schema_url as this is the same merging error. Especially, when taking into account that for entity-aware systems the users should anyway not rely on the resource schema_url.
Second example
What should we do when all entities within E have the same schema_url which is blank and the resource has some non-blank schema_url? Again the "Merging Entities into a Resource" says that the resource should have the schema_url set to blank. But for the existing stable Merge algorithm this would rather fall into:
opentelemetry-specification/specification/resource/sdk.md
Lines 94 to 95 in 8db7da2
so probably it would be better if the resource keeps its original schema_url.
There was a problem hiding this comment.
I think the previous version specification actually represents a bug.
Think about the meaning of schema url here. It tells you the schema attributes follow. It should only be set if you know all attributes follow it.
With entities we allow subgroups of attributes to follow different schema urls. If they are not all the same you cannot keep the schema url. Previously this would have led to the entire resource being rejected or the xhema url dropped. That's poor behavior when schema url mismatch between resource detectors is common as resource detectors do not all live on the sdk itself but can be extended by users.
The behavior your suggesting would lead to invalid schema url tagged on resources and in my opinion that's more of an issue here.
Take an example -
- service resource detector @ semconv version A
- cloud resource detector from dependency @ semconv version B
With entities these merge cleanly and schema url is preserved inside entity_ref. Since schema url is different - you cannot write a schema url on resource.
There was a problem hiding this comment.
A few more direct answers -
First example
What should we do when all entities within E have the same (non-empty) schema_url and the resource (non-empty) schema_url is different? The current version says that it should replace the schema_url which can be different. In my opinion, this behavior is in conflict with
...
I do not think we should specify what to do with the schema_url as this is the same merging error. Especially, when taking into account that for entity-aware systems the users should anyway not rely on the resource schema_url.
The Entities SIG considers the fact this is a merging error a BUG in the specification that prevents real life usage of resource detectors. In a survey of implementation, we found that if we had actually set SchemaUrl on Resource appropriately in resource detectors, we'd be hitting this incompatible merge all the time - or any time a user creates their own with a valid SchemaURl all the default resource detectors would start to conflict with them.
So - FIRST - we have to agree that once Entities are in play - any interaction with SchemaURL is best effort to give a reasonable answer to what Schema a Resource has. Rather than preserving compatibility with Resource - we should make sure the schema URL in resource can accurately be used to validate the telemetry and understand the attirbutes there-in.
SECOND - One major goal of entities is to fix this bug in the merge algorithm, so the "open" aspect of Resource Detection works - i.e. our default / out of the box detection can work with custom and can provide valid Schema URLs.
Second example
What should we do when all entities within E have the same schema_url which is blank and the resource has some non-blank schema_url? Again the "Merging Entities into a Resource" says that the resource should have the schema_url set to blank. But for the existing stable Merge algorithm this would rather fall into:
...
so probably it would be better if the resource keeps its original schema_url.
This is where you need to go back to the goal of what Schema URL is meant to be and do. If you keep the SchemaURL in this case, you're claiming that a bunch of Entities - which SHOULD be schema aware, are all under a particular schema. This would lead to weaver live-check errors about not matching a schema instead of telling you you don't have a schema defined. (I say would because I don't think we'll interact with Resource.schema_url at all in weaver live-check once entities are succesfully stable, as I think enforcing the existing design will lead to more breakages than usefulness)
So when you say "it probably would be better" - I'd love to understand why or what the goal is here. If the goal is to have the SchemaURL accurately depict the included attributes - then I highly disagree.
There was a problem hiding this comment.
I agree that treating two different non-empty Resource Schema URLs as a merging error with an undefined result is a bug in practice. That case is tracked in #3361. I added an updated SDK survey and a concrete proposal there that should help the resource detection even without having entities in place. The current behavior has caused recurring problems for OpenTelemetry Go.
My concern in this thread is also a different case: merging a non-empty Schema URL with an empty Schema URL.
The goal is to preserve consistent semantics and backwards compatibility for the legacy Resource Schema URL. The existing Stable Resource merge rule (https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/resource/sdk.md#merge) treats an empty Schema URL as neutral: adding schemaless attributes to a Resource with Schema URL A retains A. Go exposes this behavior directly through resource.NewSchemaless, which has been part of its stable API since v1.0.0.
With the proposed entity-aware algorithm, the same flattened attributes could instead clear Resource.schema_url solely because those attributes are represented by an Entity with an empty Schema URL. That gives Resource.schema_url different merge semantics depending on whether Entities are present. This is particularly surprising when the PR also describes that field as being retained for backwards compatibility and not used by entity-aware systems.
The underlying ambiguity is what an empty Schema URL means:
- If it means "this data makes no schema claim," it can be treated as neutral.
- If it means "the schema is unknown and may conflict," clearing the aggregate URL is safer.
The existing Resource SDK specification currently permits an empty URL in both situations. If we decide that A + empty must produce an empty URL, that changes the existing Stable Resource semantics and should be addressed consistently for Resources both with and without Entities, with explicit compatibility analysis.
When calculating the legacy Resource Schema URL, my preferred rule is to consider the distinct non-empty Schema URLs contributed by the existing Resource and the merged Entities:
- A + empty → A
- A + A → A
- A + B → empty Resource Schema URL, while preserving the merged attributes as proposed in Resource merge error for different versions #3361 (comment)
Per-Entity Schema URLs would remain authoritative for entity-aware consumers.
Would you be open to this rule? If an empty Entity Schema URL must instead invalidate A, should we change A + empty consistently for both entity-aware and non-entity Resource merges, rather than giving the same field different semantics depending on whether Entities are present?
Distinguishing “no schema claim” from “unknown or conflicting schema” may require additional state, but I would treat that as a separate issue.
There was a problem hiding this comment.
<Edit: reworded, focusing on key areas of disagreement>
I think we have two things we disagree on here, possibly?
1. We need to expand the question of what an empty schema url means both for resource and entity.
The existing Resource SDK specification currently permits an empty URL in both situations. If we decide that A + empty must produce an empty URL, that changes the existing Stable Resource semantics and should be addressed consistently for Resources both with and without Entities, with explicit compatibility analysis.
This notion of "ok to merge" is really just a hack around the bug in resource where we have one schema url to track something that has multiple pieces of instrumentation participating and needed multiple schema urls to track it.
I don't believe we have a case for "this data makes no schema claim," it can be treated as neutral. The only safe thing we can do with unknown is to claim it's unknown. Effectively there is a schema, I just haven't bothered to declared it. I understand we allow that in Resource, but that's only because there's no other way to keep resource useful. In practice this means we suggest NO tool interact with SchemaURL as it stands, because it's not "safe" or meaningful to do so. We should just drop the Schema URL.
For entity we absolutely want to have empty mean "just not specified/unknown" vs. "we don't care". That's the whole design behind it and tracking schema-urls separately, so different detectors can work together without issue.
#3361 - This is intended to be fixed by entities work - the new merge algorithm is a proposed fixed to the issue.
I think we MAY both agree on that but want to confirm.
2. The goal is to preserve consistent semantics and backwards compatibility for the legacy Resource Schema URLs
Let's be clear about this goal - for Entities we only care about full backwards compatibilty on schema-url when Resources are not using entities at all. We (Entities SIG) believe the current semantics of Resource.schema_url mean no one should be relying on the current behavior. However, if folks are trying to, we don't want them to break. However, once Entities are in the mix - we will have a schema_url that is safe to rely on, and DOES avoid merge conflicts in all the ways the Resource algorithm did not.
I did NOT want to full commit to a new resource merge algorithm change in the specification until AFTER the entities prototypes have been released and are used via opt-in flags for a while and we've seen more corner cases (beyond what the prototyping the Entities SIG has done.)
IIUC - you'd like to update the bug for the merge algorithm TODAY (particularly since, as noted in the issue, many SDKs specs are non-compliant to it).
I'd ask that we resolve that separately from this PR - for two reasons:
- Entities is still in development. The SDK features we propose here will be behind a flag. While unlikely, we may remove entities from the specification or do adjustments if we hit any kind of hard blocker. That's what "development" is for. Any change to a stable part of the Specification deserves its own PR and have consideration. This PR is about the minimal SDK features for an experiment NOT altering behavior of a core component we consider a bug.
- The merge algorithm was already thoroughly reviewed, discussed (including trade-offs of keeping schema-url) when merging the resource data model markdown. If we want to revisit - we should make changes there which is out of scope for this PR.
Would you agree that this PR should NOT change existing stable portions of the specification, only add "development" sections for new behavior that will be opt-in?
|
|
||
| ### Create | ||
|
|
||
| The interface MUST provide a way to create a new resource, from [`Attributes`](../common/README.md#attribute). |
There was a problem hiding this comment.
| The interface MUST provide a way to create a new resource. |
Attributes are not the only parameter, and we the all parameters listed below, so I suggest removing it from here.
| recorded in the emitted resource. If the `schema_url` parameter is unspecified | ||
| then the created resource will have an empty Schema URL. | ||
| - **Status**: [Development](../document-status.md) since 1.60.0 - `Entities` (optional): | ||
| Specifies the [entities](data-model.md) that |
There was a problem hiding this comment.
Is this meant to link to Resource data model doc or Entities data model doc?
pellared
left a comment
There was a problem hiding this comment.
Approving given @tigrannajaryan comments are resolved.
My concerns related to the merging algorithms should be better addressed separately.
Co-authored-by: Robert Pająk <[email protected]>
Fixes #5200
Changes
mergeto require the data model merge algorithm (or equivalent) when entities exist.For non-trivial changes, follow the change proposal process.
Prototypes:
NOTE: Did not update compliance matrix as I wasn't sure how to mark things as experimental
CHANGELOG.mdfile updated for non-trivial changes[chore]in the PR title to skip the changelog check