OTEP: Allow multiple Resources in an SDK

[jsuereth]

Alternative to #4316 -

We propose allowing {Signal}Provider to be exended with additional Entity, instead of allowing Resource to be mutable as a forward path for modelling "session" in browsers/devices. Additionally, we believe this will help with multi-tenant telemetry use cases.

E.g. This OTEP proposes the following

const sessionEntity: Entity = discoverSessionEntity();
// SDK has created an isolated meterprovider for this session.
const meterForSession = meterProvider.forEntity(sessionEntity).getMeter("my meter"); 
const latencyForSession = meterForSession.getHistogram("latency", ...);
latencyForSession.record(...); // Record latency against session.

instead of:

const meter = meterProvider.getMeter("my meter");
const latency = meter.getHistogram("latency", ...);
const sessionEntity: Entity = discoverSessionEntity();
// On resource mutation, metrics must be flushed and/or new allocation for all metrics created.
entityProvider.addOrReplace(sessionEntity);  
latency.record(...); // Record latency against session.

[dyladan]

I think this concern is relevant to any case where you need to begin collecting telemetry before an async resource detection is resolved. Even in languages which can block, it may be necessary in situations like lambda functions to begin immediately collecting telemetry without blocking startup in order to prevent adding latency to user applications.

[jsuereth]

True - I think it's just IMPOSSIBLE in today's specification where you can't block.

I'll rephrase

[kamphaus]

in the order provided does this refer to the order in the detectors list parameter?

[jsuereth]

Yes - will clarify

[kamphaus]

Should this also include the transform processor?

[jsuereth]

These are just examples, but YES - I think anything which interacts with OTTL is in-scope to be looked at.

[thompson-tomo]

For me another future use case would be a way to describe connections between scopes. For instance this instrumentation scope is a continuation of session xyz which is a use case for browser/mobile.

This also applies to cicd where a pipeline task run scope could be a child of the pipeline run scope.

Potentially Having this described in the messages makes it easier for vendors to implement as well as enabling other signals/sigs to leverage it due to the generic approach.

[tigrannajaryan]

Do I understand it correctly that every time an entity changes (including if only descriptive attributes change) you need to re-obtain a new Meter/Tracer? Or do we allow the entity to mutate and that mutation is reflected in subsequent exporting of data exported via existing Meter/Trace?

[jsuereth]

So if the identity of the entitty is different, it's a different entity. You need a different Meter/Trace by identity.

What to do about descriptive attributes is an open question. I'm inclined to not update them right now, as I think this is what the Entity-Signal should be doing going forward, and Entities used in Resource or scope should only be leveraging descriptive attributes that are "unlikely to change, but valuable".

[tigrannajaryan]

I am confused by the usage of word "tenant" here. Do you refer to instrumentation libraries as tenants here?

[jsuereth]

No, a tenant would be something beyond that.

E.g. let's look at older Java Application Servers. One AppServer may have multiple Applications.

The instrumentation library may be for "jboss" or "tomcat" or whatever the app server is. The tenant would be the application itself. I'd instantiate one scope for each tenant vs. one for the entire app server in this world.

[tigrannajaryan]

Does this OTEP imply removing EntityRefs from the Resource in proto?

[tigrannajaryan]

I like this direction.

[dashpole]

Why does this need to involve a data model change? Couldn't we just emit multiple resources?

[jsuereth]

Great question. I'll actually add it to open questions.

Pros of data model change:

• Entity used in Scope can leverage context of Resource (e..g a process in the scope of a host), as entities only have locally-unique ids.
• We have less repeated data in OTLP
• We preserve the Resource -> InstrumentationScope -> Signal model for {Signal}Provider -> {Signal} -> {operation} we have in our API today.

Cons of data model change:

• Many implementations ignore scope today, and its usage for identity is still relatively new to the data model. This would increase the need for them to interact with scope.
• Entity information would be split between resource + scope. When would two metric streams be the same, e.g.
  • a Resource with Entity A + B and empty Instrumentation scope
  • a Resource with Entity A and Instrumentation Scope with Entity B

I'm not sure I have strong opinion here, but I still lean towards the data model change.

[dmitryax]

I'm also concerned about the additional complexity introduced in this OTEP. From the user’s perspective, recreating entities from EntityRefs is already not straightforward. Adding yet another source to retrieve them creates even more overhead for processing entities in the Collector and reconstructing them on the backend. I would therefore favor a solution that does not require adding EntityRef to the Instrumentation Scope.

[smith]

Just a few typo suggestions. There's a lot here but it's an understandable summary of the proposal.

[smith]

I think something got lost here,

however many implementations do not yet respect InstrumentationScope [and] lose...

[smith]

Why does this one have a space and the other two do not?

[jsuereth]

Probably my fat fingers hitting space by accident, will fix :)

[jsuereth]

Does this OTEP imply removing EntityRefs from the Resource in proto?

No. EntityRef in resource would interact with Entity on InstrumentationScope, and we need to sort out how.

[dyladan]

If there is an entity on the scope, is there a specific implied relationship to the entities on the resource? In the call Thursday you used “runs-on”, but there could be a more generic way to phrase it that makes it more clear to everyone when you should use a scope attribute? “Nested-within” is sort of what i’m going for, but i can’t think of a good generic relationship.

[jsuereth]

I agree there's some term here. "runs-on", "in-context-of" or "nested-within" are all options. Yes - there's some kind of implicit relationship here.

[dyladan]

If an instrumentation creates a scope with an entity, how do they know if there is already that entity on the resource? It may be impossible for the instrumentation author to know if they are following this restriction or not.

For example, an instrumentation which monitors memory usage wants to ensure there is a process entity to report against and calls meter.for(processEntity) but the top-level resource detector may or may not have already added a process entity.

[jsuereth]

That's the open question.

My naive initial proposal is - that would turn into a failure. We basically have two options:

1. IT's a failure, we consider that use case problematic
2. We create a "smart" meter.for(entity), where any conflicts creates a new "identity" (combination of resource + scope) such that the conflicting higher-level is ignored for the lower-level. (So with Resource + Scope, the Scope entity overrides Resource, if we decide to flatten resource as @dashpole suggests, the Meter-level entity would override the global entity when reporting Resource.

I'm actually fine with either, caveat if we go with (2) I'm leaning against having Entity on Scope and instead have a 'layered' Resource in the SDK.

[dyladan]

I've been thinking about the meeting yesterday and I want to write down my understanding of what we discussed and how I'm currently thinking about it. Below in no particular order are some of the discussion points we hit.

No entity on scope

Instead of adding entity to InstrumentationScope, we discussed adding it directly to the core resource as a "layer" and reporting multiple resources with their associated telemetry. One possibility for lifetime management is by creating new providers and using the existing shutdown methods.

// this reports metrics against the "core" resource
const meterProvider = new api.metrics.MeterProvider();

// Scope a provider to a session entity
// This "layers" a new resource on top of the core resource using the existing merge rules
const sessionEntity = getEntityForSession();
const sessionMeterProvider = meterProvider.for(sessionEntity);

// TODO: can providers be arbitrarily layered? e.g
// const pageViewMeterProvider = sessionMeterProvider.for(getPageViewEntity());

// Use the scoped provider to report metrics against the session entity
sessionMeterProvider.getMeter('example-meter').createCounter('example_counter').add(1);

// use the core provider to report metrics which are not specific to the session
meterProvider.getMeter('example-meter').createCounter('example_counter').add(1);

Does the provider need to listen to lifetime events?

The current proposal assumes the *Provider receives the EntityInitialized event and does something. It's not clear to me what that something is intended to be. The current specification requires processors to be called synchronously with API method invocations. For example SpanProcessor.onStart() is invoked synchronously with tracer.StartSpan(). Because of this, any spans started before the resource is resolved still need to have some sort of resource attached.

In JS this is solved by allowing individual attributes of the resource to be a Promise<AttributeValue>. It is incumbent upon the processor to ensure all resource attributes are resolved before first export (by awaiting resource.waitForAsyncAttributes(). Accessing attributes before async resource is resolved results in a logged warning. Resources are merged together in the order their detectors are configured. One drawback of the current strategy is that you have to know all possible attribute keys in advance. This could be improved by allowing the key to also resolve asynchronously.

---

If both above suggestions are accepted, using provider.for() to merge entities and allowing async resource attributes, I believe there may be no need for a ResourceProvider or ResourceInitializer.

1. Resources are merged in the SDK before being passed to the *Provider constructors, async resource attributes continue to resolve even after this occurs
2. New entities, including potentially async attribute values, are layered on the resource using provider.for(...).
3. The export pipeline ensures all resource attributes are resolved before any export. If resource is resolved, wait is a noop. If a new entity was layered on top of the core resource with async attributes, this mechanism continues to work.

This is actually how the JS entity prototype currently works, except that *Provider.for is not yet implemented.

[dyladan]

EntityProvider is not defined anywhere?

[dyladan]

The name ResourceInitializer implies it handles only initialization and not the rest of the lifetime. Do we want to consider a name like ResourceManager to leave the door open for future extensions like change/add/remove entities?

type ResourceStatus = 'detecting' | 'initialized';

type ResourceChangeCallback = (
  resource: Resource,
  status: ResourceStatus
) => void;

interface ResourceProvider {
  readonly status: ResourceStatus;
  onResourceInitialize(callback: ResourceChangeCallback): void;
  onResourceChange(callback: ResourceChangeCallback): void;
}

[github-actions]

This PR was marked stale due to lack of activity. It will be closed in 7 days.

[github-actions]

This PR was marked stale due to lack of activity. It will be closed in 7 days.

[github-actions]

Closed as inactive. Feel free to reopen if this PR is still being worked on.

[reyang]

What should the API/SDK do if this is not being followed? Consider rephrase it to "the API MUST" or "the SDK MUST" so the requirement is clear.

[reyang]

What does this mean? Is "flush a child provider" a defined behavior?

[dashpole]

I think we should have this return an API provider and not an SDK provider (it is unclear which is meant here, and the restrictions below make it seem like it is an SDK provider). This is required to prevent the API's For Entity operation from depending on the SDK.

That would remove some of the more confusing restrictions below, since the API does not include ForceFlush or Shutdown.

[jsuereth]

Agree, however it may be impractical to do so in code.

One major concern I have here is that we need something to "close" or otherwise release the "sub" provider so that memory can be reclaimed. This originally proposed using the shutdown method for that purpose. We may need to do something the API for this.

[dashpole]

One major concern I have here is that we need something to "close" or otherwise release the "sub" provider so that memory can be reclaimed. This originally proposed using the shutdown method for that purpose. We may need to do something the API for this.

My proposal was to model this after metric callback registrations, and return a mechanism to "UnBind" the entity binding. I'm a bit hesitant to call this shutdown, so that it isn't confused with the actual shutdown.

[dashpole]

Go prototype: open-telemetry/opentelemetry-go#7723. I took some creative liberties, but the idea of an entity-bound provider works in the Go SDK from what I can tell.

My main realization is that we can't include the Bind function in the API since it would add a dependency on resource, which is part of the SDK.

[github-actions]

This PR was marked stale. It will be closed in 14 days without additional activity.

[MrAlias]

Is this allowing multiple resources? It seems like this is still requiring one resource, but allowing entities to be associated with providers.

It seems like this title needs to be updated.

[jsuereth]

When you create a new provider with a new entity, effectively you get a new resource you're reporting against.

[dyladan]

My main realization is that we can't include the Bind function in the API since it would add a dependency on resource, which is part of the SDK.

@dashpole can you add this comment somewhere on the diff so we can keep track of it as a thread? If this is a deal breaker it is a big problem for the OTEP as written because instrumentations are expected to be able to bind Entities to *Providers with a dependency on only the API. The bind function need only accept an Entity, not a Resource. The Resource and how the Entity is merged into it is considered to be an SDK detail not exposed via API. Is it not possible to add a bind method to the API which accepts a new type?

[carlosalberto]

Approving this after reviewing the Java POC, where Entity is defined at the API level, rather than the SDK (unlike Resource, that is).

[dyladan]

JS prototype now up to date open-telemetry/opentelemetry-js#6357

[joaquin-diaz]

Suggested change

	deisgned to allow aggregation / batching of data at any point.
	designed to allow aggregation / batching of data at any point.

[joaquin-diaz]

Does this mean that a Parent Provider needs to keep track of its child Entity Bound Providers?

[joaquin-diaz]

What happens when the Entity is no longer valid? For example, you mention Session could be an entity in a Browser, but if the session ends, do you also need to close/flush the bounded provider? And related to https://github.com/open-telemetry/opentelemetry-specification/pull/4665/changes#r2858808978, do you also need to tell the parent provider that the child entity is no longer needed?

[martinkuba]

If I understand correctly, this proposal works when the caller of forEntity() is the only emitter of the telemetry bound to that entity. How would this work for entities that should be applied to all telemetry (all instrumentations, and user application)? Browser sessions fall into this category.

In addition, when a browser session is rotated, the new session entity needs to be updated for all instrumentations. Instrumentations (typically) use the globally-registered providers. They would all either need to be notified when it changes, or the global provider itself would need to allow the entity to be updated. Is this use case purposefully excluded from the proposal? Would this be part of the API or SDK?

How would this work with multiple entities? Is the intent to chain forEntity() calls?

[jack-berg]

Approve, but having learned that the browser SIG doesn't think this solves their problem, I don't think this is high enough priority to act on right now.

[reyang]

I support the direction of this OTEP. I've left several comments, but these are not blocking, we can sort these out when we actually hammer out the specification.