Skip to content
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

Closed
alanwest opened this issue Mar 23, 2022 · 6 comments
Closed

Clarify metric view behaviors #2433

alanwest opened this issue Mar 23, 2022 · 6 comments
Assignees
Labels
spec:metrics Related to the specification/metrics directory

Comments

@alanwest
Copy link
Member

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.

meterProvider.AddView(instrumentName: "name", metricStreamConfig { name: "newname" })
meterProvider.AddView(instrumentName: "name", metricStreamConfig { name: "newname" })
counter = meterProvider.GetMeter("mymeter").CreateCounter("name")
counter.Add(10)
meterProvider.ForceFlush()

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).

meterProvider.AddView(instrumentName: "name", metricStreamConfig { tags: ["key1"] })
meterProvider.AddView(instrumentName: "name", metricStreamConfig { tags: ["key2"] })
counter = meterProvider.GetMeter("mymeter").CreateCounter("name")
counter.Add(10, key1: value1, key2: value2)
meterProvider.ForceFlush()

Two views matching one instrument that define different histogram bounds but result in name conflict.

Similarly to the tags scenario but setting histogram bounds.

meterProvider.AddView(instrumentName: "name", metricStreamConfig { explicitHistogramBounds: [10.0, 20.0] })
meterProvider.AddView(instrumentName: "name", metricStreamConfig { explicitHistogramBounds: [5.0, 15.0] })
histogram = meterProvider.GetMeter("mymeter").CreateHistogram("name")
histogram.Record(10)
meterProvider.ForceFlush()

The interpretation of the above scenarios was primarily influenced by the following wording in the View API spec

For each View, if the Instrument could match the instrument selection criteria:
Try to apply the View configuration. If there is an error or a conflict (e.g. the View requires to export the metrics using a certain name, but the name is already used by another View), provide a way to let the user know (e.g. expose self-diagnostics logs).

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.

meterProvider.AddView(instrumentName: "name", metricStreamConfig { name: "othername" })
meter = meterProvider.GetMeter("mymeter")
counter1 = meter.CreateCounter("name")
counter2 = meter.CreateCounter("othername")
counter1.Add(10)
counter2.Add(10)
meterProvider.ForceFlush()

This wording from the View API spec seems to suggest that the conflict should flow through.

If the Instrument could not match with any of the registered View(s), the SDK SHOULD enable the instrument using the default aggregation and temporality.


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.

@alanwest alanwest added the spec:metrics Related to the specification/metrics directory label Mar 23, 2022
@jack-berg
Copy link
Member

Roughly speaking, my original interpretation of the specification is that each view added should always result in a new metric stream regardless of conflict.

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.

@cijothomas
Copy link
Member

cijothomas commented Mar 23, 2022

Roughly speaking, my original interpretation of the specification is that each view added should always result in a new metric stream regardless of conflict.

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.

@alanwest
Copy link
Member Author

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.

Try to apply the View configuration. If there is an error or a conflict (e.g. the View requires to export the metrics using a certain name, but the name is already used by another View), provide a way to let the user know (e.g. expose self-diagnostics logs).

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?
My read is counter2 should not be ignored since it didn't match a view. A conflicting stream will result in this case.

@jack-berg
Copy link
Member

@jmacd as the author of the metrics identity PR, it would be good to know your thoughts on this.

@jmacd
Copy link
Contributor

jmacd commented Apr 1, 2022

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).

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.

My read of this part in the specification:

* For each View, if the Instrument could match the instrument selection criteria:
  * Try to apply the View configuration. If there is an error or a conflict (e.g. the View requires to export the metrics using a certain name, but the name is already used by another View), provide a way to let the user know (e.g. expose [self-diagnostics logs](https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/error-handling.md#self-diagnostics)).

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.

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).

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. datamodel.md reads:

Producers SHOULD prevent the presence of multiple `Metric` identities
for a given `name` with the same `Resource` and `Scope` attributes.
Producers are expected to aggregate data for identical `Metric`
objects as a basic feature, so the appearance of multiple `Metric`,
considered a "semantic error", generally requires duplicate
conflicting instrument registration to have occurred somewhere.

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 Metrics and if the attributes were to overlap they would break the single-writer rule. I want to allow this to happen, but I don't want an implementation to go as far as inserting additional logic to combing two Metrics produced by non-overlapping Views for the same metric identity.

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.

Two views matching one instrument that define different histogram bounds but result in name conflict.
Similarly to the tags scenario but setting histogram bounds.

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.

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 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?

@alanwest
Copy link
Member Author

Resolved in #2462.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
spec:metrics Related to the specification/metrics directory
Projects
None yet
Development

No branches or pull requests

5 participants