Replace the view implementation in the metric SDK

[MrAlias]

There are design and usability issues with the current view implementation. This issue introduces a proposal to replace the current view implementation with one that addresses these issues.

Issues

View package is used as a namespace for a sprawl of options

It seems the main motivation for the view package is to encapsulate the large number of configuration options that exist, and those that could exist. This choice was to help prevent the pollution of the sdk/metric code API and its documentation. However, it only splits the issue to another package.

Instead of having a sprawl of options, ideally, view definitions should be defined concisely.

View package includes unrelated types.

The included Instrument and InstrumentKind types refer to metric instrument concepts, not view concepts. They are only included in the package to avoid an import cycle.

Ideally, these types would live in the sdk/metric package.

Creation of a view returns an error

The view.New function is declared as follows.

func New(opts ...Option) (View, error) {

Because this function is declared to return not only a View, but also an error it cannot be used in-line by users during the SDK setup. For example:

view, err := view.New(/* user options */)
if err != nil {
// Error handling.
}
NewMeterProvider(/* use view here */)

Ideally, the view could be defined in-line with the MeterProvider. E.g.

NewMeterProvider(WithView(NewView(/* user options */)))

This would be possible if a design can be determined that would remove the two explicit errors the New function returns.

The implicit errors, those sent to the logging system by options, are not impediments to resolving this issue.

Multiple user options of the same kind are dropped

The view.New function docs read:

// New returns a new configured View. If there are any duplicate Options passed,
// the last one passed will take precedence. The unique, de-duplicated,
// Options are all applied to the View.

This seems like appropriate behavior, but is also means that a user needs to read this documentation to understand how multiple options are handled. If they want to match instruments with name foo and bar so they pass both as options to the view it will cause frustration when both are not matched.

If it were not possible for a user to pass multiple values for a match or replacement criteria by design they would understand without reading docs or frustration that they need to use multiple views.

The current view is not extendable by a user

If a user wants to do something unique with a View outside of the provided options they are not enabled. This may seem ideal as it restricts users to only create views defined by OTel. However, this project should not be about restricting users. It should be about providing useful packages that help them achieve what they want.

It would be a benefit to the project if Views could be defined in a way that allows users to implement any idea they have, but also provide users with a way to create views directly based on what OTel prescribes.

Proposal

A complete example of this proposal can be found here

Move Instrument and InstrumentKind to the sdk/metric package

Both types can be moved to the sdk/metric package. The import cycle that prevented this is resolved in the rest of the proposal.

The Instrument type can be extended to include all properties of a new instrument. That way users can match with complete context of the instrument being created:

// Instrument describes properties an instrument is created with.
type Instrument struct {
	Name string
	Description string
	Kind InstrumentKind
	Unit unit.Unit
	Scope instrumentation.Scope
}

An added benefit of this complete definition is that the function parameters of the pipeline can be unified with this type. See the example for how this is done.

Add a Stream type to the sdk/metric package to describe the metric data stream

// Stream describes the stream of data an instrument produces.
type Stream struct {
	// Instrument describes the instrument that created the stream.
	Instrument

	Temporality metricdata.Temporality
	Aggregation aggregation.Aggregation
	AttributeFilter attribute.Filter
}

This type is a representation of the data stream from the OTel specification.

Replace the view.View with a View func in sdk/metric package

Currently a View is defined as a struct type that only conveys the configuration output of view.New. It is not configurable outside of the view package.

If instead we define a view as a function translating an instrument definition into a data stream definition, a user will be enabled to define view configuration as they see fit and we are still able to correctly create pipelines. For example:

// View is an override to the default behavior of the SDK. It defines how data
// should be collected for certain instruments. It returns true and the exact
// Stream to use for matching Instruments. Otherwise, if the view does not
// match, false is returned.
type View func(Instrument) (Stream, bool)

With this definition a user is able to add whatever matching conditions they need, even if they exist outside what OTel specifies, and create data streams in the context of the instrument that is being created.

Alone, however, this would require users to recreate common views (i.e. renames, aggregation setting, description updates), and it doesn't provide the functional niceties specified by OTel (wildcard name matching). These issues are addressed by including a NewView function to create a View with these niceties.

Define NewView to create a View based on OTel specification

// NewView returns a View that applies the Stream mask for all instruments that
// match criteria. The returned View will only apply mask if all non-zero-value
// fields of criteria match the corresponding Instrument passed to the view. If
// no criteria are provided, all field of criteria are their zero-values, a
// view that matches no instruments is returned.
//
// The Name field of criteria supports wildcard pattern matching. The wildcard
// "*" is recognised as matching zero or more characters, and "?" is recognised
// as matching exactly one character. For example, a pattern of "*" will match
// all instrument names.
//
// The Stream mask only applies updates for non-zero-value fields. By default,
// the Instrument the View matches against will be use for the returned Stream
// and no Aggregation or AttributeFilter are set. If mask has a non-zero-value
// value for any of the Aggregation or AttributeFilter fields, or any of the
// Instrument fields, that value is used instead of the default. If you need to
// zero out an Stream field returned from a View, create a View directly.
func NewView(criteria Instrument, mask Stream) View

Adding this function allows for the matching of instrument properties, including wildcard name matching, and the replacement functionality defined by the OTel specification. It also facilitates common matching/replacement views similar to the existing view.New.

Why not define this function to accept Options? Accepting options makes sense as a way to allow forward compatibility, when new options need to be added new functions returning new options can be added in a backwards compatible way. They also prevent the user from having to pass empty arguments as parameters when no options are desired.

To the latter point, when a user is using this convenience function to create a View they should never be passing "no options". If they do not pass a match criteria or mask, the view is effectively a disablement or no-op. Neither of these is likely what the user wants to do. It is expected that the common use of this function will be by users that want to match something and set something. Given this expected use, designing for the no-option possibility here is non-ergonomic.

As for the extensibility of future options, both the Instrument and Stream are struct that can be extended. The zero-value of fields for these structs are ignored so adding new fields will behave the same as the previous non-included-field version.

As an added benefit of fully specifying each allowed parameter, there is no possibility for a user to provide duplicate options. It becomes clear by design that only one name, description, instrument kind, etc. is matches per created View. This is something that only was understood in documentation with the option approach.

This added benefit of single fully specified match criteria also removes the existing error of view.New where an exact and wildcard name match is asked for.

As for the other existing error, where a user does not provide any matching criteria, it is translated into a returning a view that never matches. As mentioned above, the inherent design of the function discourages this use.

Having both possible explicit errors removed, the function no longer needs to return an error. It is now able to be included in-line with MeterProvider creation options.

The existing view.New also allows for logged errors when it is passed aggregations that are misconfigured. This new function can use this approach to error handling for that error type. Additionally, it can use that approach for any additions to the parameters it receives that result in errors.

Deprecate the view package in favor of implementation in sdk/metric

• Deprecate sdk/metric/view.Instrument in favor of sdk/metric.Instrument
• Deprecate sdk/metric/view.InstrumentKind in favor of sdk/metric.InstrumentKind
• Deprecate sdk/metric/view.View in favor of sdk/metric.View
• Deprecate sdk/metric/view.New in favor of sdk/metric.NewView
• Update sdk/metric to stop using all deprecated types

Tasks

How to split the proposal into reviewable PRs.

• Add View, NewView, Instrument, Stream, and InstrumentKind to sdk/metric with unit tests
  • Add View, NewView, Instrument, Stream, and InstrumentKind to sdk/metric with unit tests #3459
• Update sdk/metric to use View, NewView, Instrument, Stream, and InstrumentKind from sdk/metric
  • Replace view use from sdk/metric/view to sdk/metric #3461
• Add example tests for NewView and View.
  • Add view example tests #3460
• Deprecate the view package
  • Deprecate the sdk/metric/view package #3476

[MrAlias]

I could also see having the NewView function just be a MeterProvider option function, similar to WithView, once #3387 lands.

[MrAlias]

I could also see having the NewView function just be a MeterProvider option function, similar to WithView, once #3387 lands.

Thinking about this for a bit, it doesn't seem advantageous. If instead of NewView we included:

func WithView(criteria Instrument, mask Stream) Option

There would be two main issues:

1. User defined View functions would not be usable, there would be no option function to pass them to NewMeterProvider.
   • An additional option function could be added to accept the View type. However, this would have an equivalent API surface area without the extensibility (discussed in the second issue). Furthermore, it would confuse users as to how they should register views. There would be two closely related option functions and the understanding of which one they should use would need background understanding.
   • We could also just not add a View type to the sdk/metric package, views would only be managed by the WithView option function. However, this would be short-sighted. It would remove the user extensibility and customization, and leave many of the identified issues with the current implementation intact.
2. The created View of WithView could not be usable by other user created views, they would be un-composable. If a user wants to extend a view created by NewView they could. If we used WithView they would not have access to the View and could not do so.

[MrAlias]

This would also resolve/supersede #3199

[MadVikingGod]

This added benefit of single fully specified match criteria also removes the existing error of view.New where an exact and wildcard name match is asked for.

As for the other existing error, where a user does not provide any matching criteria, it is translated into a returning a view that never matches. As mentioned above, the inherent design of the function discourages this use.

You missed another error of the NewView, when you supply a wildcard and a rename.

SDK SHOULD NOT allow Views with a specified name to be declared with instrument selectors that may select more than one instrument (e.g. wild card instrument name) in the same Meter. ¹

Why define the view as a func and not an open interface? A user can easily implement both, but if it's a func then there is never a chance for us to extend it via interface casts (see WriterTo and its use in the std library)

Should we consider similar open designs for other portions of this API, specifically readers?

Footnotes

1. https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/sdk.md#view ↩

[MrAlias]

This added benefit of single fully specified match criteria also removes the existing error of view.New where an exact and wildcard name match is asked for.
As for the other existing error, where a user does not provide any matching criteria, it is translated into a returning a view that never matches. As mentioned above, the inherent design of the function discourages this use.

You missed another error of the NewView, when you supply a wildcard and a rename.

SDK SHOULD NOT allow Views with a specified name to be declared with instrument selectors that may select more than one instrument (e.g. wild card instrument name) in the same Meter. 1

Gotcha, this sounds like a good use-case for the error logging and drop approach:

The existing view.New also allows for logged errors when it is passed aggregations that are misconfigured. This new function can use this approach to error handling for that error type. Additionally, it can use that approach for any additions to the parameters it receives that result in errors.

Similar to what we already do for invalid aggreagtions.

[MrAlias]

Why define the view as a func and not an open interface? A user can easily implement both, but if it's a func then there is never a chance for us to extend it via interface casts (see WriterTo and its use in the std library)

The supported parameters and return value are extendable. The functional significance of the view itself, transforming a instrument definition to a stream definition, seems complete to me.

What functionality do you expect we will need to add?

Adding a new functionality to a view in the future that this cannot support would be one that transforms a different set of parameters into a different result. In that situation, it seem appropriate, and clearer to a user, to add a new function type.

[MrAlias]

Should we consider similar open designs for other portions of this API, specifically readers?

Not sure I follow the suggestion. Should we make a Reader a function? I don't think that is appropriate given the specification defines it as something that performs multiple functions.

[MrAlias]

@reyang @jmacd to your knowledge will it be non-compliant with the specification if we allow users to define their own view functions as described here?

They would still have the "guard-rails included" option of the NewView function to create views as prescribed by the specification, but was it the intention of the specification to restrict users further?

Based on the current view specification it didn't seem to be the case, but it was raised in today's Go SIG meeting that we should check.

[reyang]

@reyang @jmacd to your knowledge will it be non-compliant with the specification if we allow users to define their own view functions as described here?

If my understanding is correct, this is a general question "if a concrete implementation chooses to provide some extra thing that is not defined in the specification, is it considered to be spec-compliant or not?". @MrAlias is my understanding correct or you're looking for something else?

[MrAlias]

@reyang @jmacd to your knowledge will it be non-compliant with the specification if we allow users to define their own view functions as described here?

If my understanding is correct, this is a general question "if a concrete implementation chooses to provide some extra thing that is not defined in the specification, is it considered to be spec-compliant or not?". @MrAlias is my understanding correct or you're looking for something else?

Yeah, I think that is a fair restatement. 👍

[reyang]

This is an interesting topic, I don't think we've ever made it explicit.

Here is my personal thinking - the specification defines the minimum bar for an implementation to be considered compliant. Providing extra features that are not required by the specification should not be considered as violation, unless the specification explicitly prohibits that.

Having said that, an implementation should make careful judgements whether a certain addition should be aggressive (just go and do it in a certain implementation, without going through the specification) or conservative (bring it to the specification, get consensus whether this is common or not, then do it across implementations or not). This would avoid an implementation to go too quickly and introduce something that later got reintroduced to the spec in a different way. This requires good judgement from the project maintainers.

One good example is https://github.com/open-telemetry/opentelemetry-dotnet/issues/new?assignees=&labels=enhancement&template=feature_request.md, the issue template specifically encourages people to consider if the ask is a .NET specific thing or a language agnostic thing, if it is not specific to .NET, folks are asked to bring this to the specification SIG.

Please let me know if this is helpful, or you would expect the TC to make it easier for maintainers. I can bring this to the TC discussion if there is high demand.

[MrAlias]

Thanks for the guidance @reyang. I think this proposal is advantageous for the reasons you outlined. It provides NewView to ensure that users are able to paint in the lines of what OTel specifies, and since we expose the intermediary View they can still use primitives that suite their need.

Looking at other implementation, I think this proposal would bring us closer to what .NET actually does. Having both:

https://github.com/open-telemetry/opentelemetry-dotnet/blob/88c1864cb79ba6bf9623db6a77f4383c38799197/src/OpenTelemetry/Metrics/Builder/MeterProviderBuilderBase.cs#L142

and

https://github.com/open-telemetry/opentelemetry-dotnet/blob/88c1864cb79ba6bf9623db6a77f4383c38799197/src/OpenTelemetry/Metrics/Builder/MeterProviderBuilderBase.cs#L166

Both being similar to NewView, and:

https://github.com/open-telemetry/opentelemetry-dotnet/blob/88c1864cb79ba6bf9623db6a77f4383c38799197/src/OpenTelemetry/Metrics/Builder/MeterProviderBuilderBase.cs#L192

Being equivalent to a user defining a View function directly.

[reyang]

These OpenTelemetry .NET abstractions are currently marked as internal, which means it is not publicly exposed to the user.

[MrAlias]

These OpenTelemetry .NET abstractions are currently marked as internal, which means it is not publicly exposed to the user.

Ah, gotcha.

[MrAlias]

@reyang are these the public derfinitons?

https://github.com/open-telemetry/opentelemetry-dotnet/blob/6ff512cf6020432ec8f4cefbd107ea89428c77bc/src/OpenTelemetry/Metrics/Builder/MeterProviderBuilderExtensions.cs#L89-L151

Sorry, my .NET understanding is slim. 😞

[reyang]

@reyang are these the public derfinitons?

https://github.com/open-telemetry/opentelemetry-dotnet/blob/6ff512cf6020432ec8f4cefbd107ea89428c77bc/src/OpenTelemetry/Metrics/Builder/MeterProviderBuilderExtensions.cs#L89-L151

Sorry, my .NET understanding is slim. 😞

Yep, the methods are public and the class which contains these methods is also public.

[reyang]

This doc might be helpful: https://github.com/open-telemetry/opentelemetry-dotnet/tree/main/docs/metrics/customizing-the-sdk#view

[MrAlias]

From the SIG meeting today: The plan is to move forward with this proposal and start submitting PRs.

[jmacd]

We discussed this in the TC meeting yesterday and wrote down these considerations.

In this case, my understanding of the group's position is that we should file an issue in the specification repository for other language groups to see before embarking on major extensions. This will help us avoid conflicting extensions across the community.

At the same time, we support making extensions, but advise keeping them out of the standard OTel namespace. It means that support for user-defined view behavior sounds fine if it can be kept in a package where it will not conflict with future specified OTel behaviors. Maybe in this case, use sdk/metric/extension/* packages for the hooks you want to offer?

CC: @reyang @jsuereth

[reyang]

Based on the TC discussion, I've created this PR open-telemetry/opentelemetry-specification#2968.