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

Should the SDK API constrain the Readers that can be used? #3085

Closed
MadVikingGod opened this issue Aug 11, 2022 · 3 comments · Fixed by #3387
Closed

Should the SDK API constrain the Readers that can be used? #3085

MadVikingGod opened this issue Aug 11, 2022 · 3 comments · Fixed by #3387
Assignees
Labels
area:metrics Part of OpenTelemetry Metrics enhancement New feature or request pkg:SDK Related to an SDK package

Comments

@MadVikingGod
Copy link
Contributor

Background

Currently, we require that the Readers provided to the WithReader Option be comparable. We check that the two readers we create are comparable, but if a user embeds a Reader in a non-comparable struct WithReader will panic. An example of how to trigger this can be found here.

This type of embedding will probably be used in the prometheus exporter.

Problem Statement

Should we further constrain our option to prevent users from supplying a non-comparable reader? This would likely be run into by exporter developers and hopefully caught by a working example.

Proposed Solution

We could create a ComparableReader interface that we constrain the WithReader with. Here is an example branch.

This would in effect turn the runtime panic into a compile-time error. You wouldn't be able to use a non-comparable Reader with the only option we have.

This would have two drawbacks:

  1. This would change the SDK API if done after the alpha is released. We are allowed to, but I would like to minimize this if possible.
  2. Because you can't cast Reader -> ComparableReader the output of New*Reader would have to change from Reader.
  • Either to a *manualReader, which has usability concerns,
  • or to a *ManualReader, note this is exporting ManualReader, which means we need to guard aginst users using a &ManualReader{}

Alternatives

  1. Accept the runtime error, and encourage exporter creators to check that it works with the SDK.
  2. Remove the comparable constraint on readers.
@MadVikingGod MadVikingGod added enhancement New feature or request pkg:SDK Related to an SDK package area:metrics Part of OpenTelemetry Metrics labels Aug 11, 2022
@MadVikingGod MadVikingGod added this to the Metric SDK: Beta milestone Aug 11, 2022
@MrAlias
Copy link
Contributor

MrAlias commented Oct 14, 2022

Closing as #3233 looks to have resolved this. The Reader implementation no longer needs to be comparable. Please reopen if this was an error.

@MrAlias MrAlias closed this as completed Oct 14, 2022
@MrAlias
Copy link
Contributor

MrAlias commented Oct 14, 2022

readers map[Reader][]view.View

@MrAlias MrAlias reopened this Oct 14, 2022
@MrAlias
Copy link
Contributor

MrAlias commented Oct 14, 2022

func newPipelines(res *resource.Resource, readers map[Reader][]view.View) pipelines {

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:metrics Part of OpenTelemetry Metrics enhancement New feature or request pkg:SDK Related to an SDK package
Projects
No open projects
Status: Done
2 participants