-
Notifications
You must be signed in to change notification settings - Fork 888
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
Clarify metric view behaviors #2433
Comments
Agree and would add that I've been thinking of view conflicts as an extension of instrument identity conflict discussed in the API. That is, if you register views that create an identity conflict, the SDK logs a diagnostic error message but allows the conflict to be propagated to exported data. |
Thanks @alanwest and @jack-berg for sharing your thoughts. The above is where I have a different understanding, thanks for bringing this up, so we can all get aligned. Per my reading - The instrument identify conflict section, part of API spec, is only meant to cover the scenario where user/library is creating identical instruments. The examples Alan shared, are all due to app owners creating Views and explicitly creating conflicts. Hence it must be treated as Views registration errors, and fail-fast if possible, else ignore the view (after doing some logging..) The app owner (who is the only person who can create views, unlike the instruments which can be created by any libraries..), must do action(s) to rectify their Views configuration. |
I like @cijothomas's interpretation. Though I think the spec could be more clear if this was the intention. The following words mention conflicts and requires SDKs to log but does not state to ignore that stream. I think this should be clarified.
The last example I provided is different from the others. I think it requires a clarification as well, but maybe somewhere in the API specification? |
@jmacd as the author of the metrics identity PR, it would be good to know your thoughts on this. |
Keep in mind that a major use-case for Views is to correct or suppress duplicate registration conflicts. So, even though the rules for stating what is a conflict from the API's perspective, the SDK has to have the flexibility to defer warning about conflicts until after views are processed. This means detecting conflicts via the resulting metric objects(name, description, unit) and semantic information (data point kind, int/float, etc). I believe we should always pass through literally whatever the View says to do for every matching instrument AND we should warn the user about conflicts in the resulting output so that Views can correct instrumentation conflicts. So for each of the scenarios below, the implied conflicting data should be produced with a warning about the conflict (unless special measures are taken to convert data, which I do not expect as standard behavior).
My read of this part in the specification:
is that both View clauses will be applied since they both match. A duplicate conflict is created, and two identical copies of the metric will be written.
I tried to avoid this matter when working on #2317, since practically speaking in this configuration there are no single-writer errors. We know that Prometheus users sometimes solve single-writer conflicts with the use of attributes this way, so I don't want to say you shouldn't do this.
In my current OTel-Go views implementation, for this scenario, the above rule would be broken. Because two Views clauses created two outputs with the same name, even though they have the same identity ignoring attributes, they are written as two IOW this will work for a user, and as long as the consumer isn't being strict about the above rule (and provided the attributes do not overlap), I would expect no real problems here. If this scenario is truly common, it's going to cause a lot of spurious warnings, but I think we have options to correct this in the future.
Same as above, I would expect two conflicting histograms to be produced with different bounds. See this passage in the data model spec, which gives us options to resolve this by for example merging the buckets (e.g., 5, 10, 15, 20 is a superset of both configurations, I would accept this outcome). Unlike the above, however, this is definitely going to create a single-writer rule violation, so the warning is never spurious here.
This is a conflict in the data model but not at the instrumentation level. This does appear to be a case where the View could fail-fast because a user-provided error (not instrumentation), except what if the instrument is created after the view is registered? |
Resolved in #2462. |
Opening this issue to help clarify some understanding of the view API specification as it relates to the recent changes regarding instrument identity from #2317. So far, I've spoken with @cijothomas (.NET) and @jack-berg (Java) - there are some differences in understandings, so bringing this to the community at large for comment.
The following scenarios reflect the discussion that the .NET SIG has had.
Two views matching one instrument resulting in conflicting name
The second view config conflicts with the first. The instrument matches both views, but the second one is ignored and only one metric stream is produces on flush.
Two views matching one instrument that filter for different tags but result in name conflict.
Same as above, the second view is ignored due to name conflict. Only one stream is produced with key1 (key2 is filtered out).
Two views matching one instrument that define different histogram bounds but result in name conflict.
Similarly to the tags scenario but setting histogram bounds.
The interpretation of the above scenarios was primarily influenced by the following wording in the View API spec
One view matching one instrument resulting in name conflict with a second instrument not matching any views
Should the conflict be allowed? Not technically a conflict due to duplicate instrument registration since the instruments are different, but the application of the view creates the conflict.
This wording from the View API spec seems to suggest that the conflict should flow through.
Roughly speaking, my original interpretation of the specification is that each view added should always result in a new metric stream regardless of conflict. That is, the scenarios above where a second view is ignored would instead result in a second metric stream with a conflicting identity. In speaking with @jack-berg he too shared this understanding.
The text was updated successfully, but these errors were encountered: