Clarify view conflicts

[jack-berg]

Related to #2443.

• Views that create instrument identity conflicts SHOULD be applied.
• Views that create semantic errors should be dropped.

[alanwest]

The other thing we discussed in this week's spec meeting was the option for SDK's to fail fast when it is known a view will cause conflicts.

There's a section right above that says:

In order to avoid conflicts, views which specify a name SHOULD have an
instrument selector that selects at most one instrument...

Could some additional more generic wording be added to this like

For all other cases where it is known that registering a view will cause a conflict, then SDKs MAY fail fast.

[jmacd]

Re-stating my approval.

[tsloughter]

"applied" means this is saying that measurements should be aggregated for all Views even if they conflict?

Assuming that is the meaning, and not that "apply" is referring to "applying configuration" and replacing the previous View in the state of Views, I don't see the purpose/benefit mentioned?

But now that I realize it also says a warning is emitted when the View is "applied" it must be that applying refers to updating the state and not applying the View against a measurement (since that would be a lot of warnings)?

In that case I'd just suggest using a different word and clarifying if it is a full replacement or a merge.

---

Re-reading and the context is, deciding what to do with a measurement. I think that implies "apply" would mean actually applying the View to the measurement and emitting a warning for every measurement seems excessive -- an additional concern to my other about why we'd want conflicting metrics.

[tsloughter]

Ok, now I realize there might even be another case this is meaning to cover, which can't be handled at configuration time -- a no name View with use of wildcards (? and *) in the instrument name match, making it unknown at the time of view creation if any of those Views it will ever match 0, 1 or N instruments.

I'd argue the spec should define that the Views are kept in the order they are registered and matching is either first or last wins in the case there is a conflict in the resulting name.

On slack it sounds like all implementations require up front View creation, but the spec reads as Views are matched to Instruments each time a measurement is taken. I wouldn't have considered this an issue -- since the end result is the same -- but now I think the spec could be written more easily and clearly by defining Instrument and View matching this way as well.

[jack-berg]

@tsloughter this PR doesn't introduce the language to "apply the View" or to log a warning if there's a conflict. That language has already been there and I think it's implied that logging doesn't refer to handling each individual measurement.

a no name View with use of wildcards (? and *) in the instrument name match, making it unknown at the time of view creation if any of those Views it will ever match 0, 1 or N instruments.

Yes. Additionally, a view which changes the name might match exactly one instrument, but the view's name might conflict with another instrument which has no matching views.

This PR is all about clarifying to ensure that SDKs behave similarly in these types of situations. It resulted from discovering that the dot net and java implementations had different interpretations.

I'd argue the spec should define that the Views are kept in the order they are registered and matching is either first or last wins in the case there is a conflict in the resulting name.

That's roughly the position that's expressed in this comment.

[tsloughter]

@jack-berg ok, yea, I see "apply" was already used. But the main change appears to be, "the implementation SHOULD apply the View and emit a warning". Ignoring whether "apply" should be used here or not, I'm still not sure what I should do when implementing this to get consistent results?

[jack-berg]

We spoke about this issue at the 4/5 Spec SIG.

@jmacd brought up the point that in instruments "MUST create a valid Instrument in every case." Suppose we were to take the position that we should ignore views that produce conflicts, and a view is registered that renames an instrument from foo to bar, and later an instrument is registered with name bar. A view has created a conflict, but it wasn't possible to know about that conflict until some later point when the instrument with name bar was registered.

The lack of ability to know that a view will produce a conflict, and the requirement that instruments must always create valid instruments that export data suggests that the most correct / consistent behavior is for the SDK to produce all views regardless of whether they produce conflicts.

There seemed to be some consensus on this thought at the SIG meeting, and this PR currently reflects that position.

[tsloughter]

Jack reached out on slack and explained everything. I get now that this simply means conflicting metrics will be reported and that the user must intervene to resolve this error.

+1 to this solution. As was noted, the fact that a conflict could result from instrument being created at runtime that conflicts with the name of a view but Instrument API requires a valid Instrument always be returned makes it really the only viable solution.

My only concern is that at least one form of overriding views is useful: Setting up a view that drops everything and a view that doesn't and matches certain instruments only.

This would work fine with this PR as is, I just wonder if it could be confusing to give the appearance that view overriding works, when it is not an override at all. May just need some docs by implementations for SDK users.

[reyang]

@jack-berg CI is failing, would you look into it?