Consider removing Resource::Merge()

[yurishkuro]

This discussion #56 (comment)

[jmacd]

I could imagine wanting to allow resources to be defined at various locations in the code. Maybe you have linked in some code that knows about Linux /proc file entries, and is able to generate detailed resource values. Another module knows about K8S and is able to generate detailed resource values. I wouldn't try to convince you that a single main() routine is going to be effective at gathering all the resource-providing modules; instead I think it should be possible to combine independently produced resource listings.

That said, I wouldn't try to define the semantics of combining overlapping resource definitions. Why not treat the resource definition as a map, with normal update semantics? most programming runtimes do not specify the order of initializers, so the programmer should avoid creating overlapping resource definitions. I don't really see a problem that Merge will help with.

[bogdandrutu]

@jmacd the way how merging is implemented (map semantics, or other strategy) will result in how this API will be used by different modules (as you mentioned k8s) and consumed by different implementation.

For example imagine that we don't define a merge API then every module will try to overwrite all the fields because some implementation may decide to keep only the last Resource. So it is very important to define how multiple Resource will be "merged".

So I feel uncomfortable to not have a well defined algorithm on how multiple resources will be merged together, but I am open to discuss if the current algorithm makes sense vs having a map like semantic or other semantics.

Talking about "map semantic" I think we have some inconsistency there:

• From Java Map.put - If the map previously contained a mapping for the key, the old value is replaced by the specified value.
• From C++ map.insert - Because element keys in a map are unique, the insertion operation checks whether each inserted element has a key equivalent to the one of an element already in the container, and if so, the element is not inserted, returning an iterator to this existing element (if the function returns a value).

Because of this I think we cannot simply say that we have "map semantics" and we actually have to define the semantic.

QQ: What is "normal update semantics"?

[jmacd]

I don't think defining map semantics is the hard part, it's that these updates happen in an undetermined order if we let them happen as a result of program initializers.

I believe the options are to:

(1) allow the ambiguity, suggest that names should be qualified by namespaces to avoid conflicts, and allow static initializers to apply global resources in unspecified order to the global tracer.
(2) force the programmer to initialize resources explicitly in the order they choose, and dictate that either first or last value wins in the case of multiple updates. I think last-value wins is a fine semantic here, but I prefer the first approach.

I'd say it's more important to talk about namespaces than about merging algorithms.

[yurishkuro]

I'd say it's more important to talk about namespaces than about merging algorithms.

+1

I prefer simplicity, and I think this Merge() function and the respective merge semantics are unnecessary complications to the spec that can be easily handled by user code / instrumentation. But perhaps I am missing some elaborate use cases.

[bogdandrutu]

@jmacd for the namespace discussion, see the current OpenCensus definition https://github.com/open-telemetry/opentelemetry-specification/blob/master/work_in_progress/opencensus/StandardResources.md

@yurishkuro Merge is important in this scenario:
If we do offer the components API for example see #10 then when you build the new Tracer from the previous one, the resource labels that you we believe that should be merged to the "parent" Tracer. Having an algorithm about how that is going to work is important to have a deterministic result.

[jmacd]

To follow up on this, I've posted an issue #76 about making resources into structured values instead of a distinguished type in the API, that would also address the issue with structured events in issue #67.

[lmolkova]

Today spec does not define any scenario where resources could be merged (it only allows to assign one resource per tracer that propagates to spans)

So having Merge API seems to serve some future scenarios. Introducing API in v1 for future scenarios that are not yet designed seems to be risky - chances are this API won't be needed or won't work as we will want it to.

So I suggest that if we keep this API in v1, we should understand, support and document some scenarios where it is needed, If we don't have them - we should remove it.

[carlosalberto]

Closing this issue as Resource is not part of API anymore. Please re-open if you think this is still important or similar ;)