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

Add sdk/metric/view structure to new_sdk/main #2817

Closed
2 tasks done
MrAlias opened this issue Apr 19, 2022 · 4 comments · Fixed by #2838
Closed
2 tasks done

Add sdk/metric/view structure to new_sdk/main #2817

MrAlias opened this issue Apr 19, 2022 · 4 comments · Fixed by #2838
Assignees
Labels
area:metrics Part of OpenTelemetry Metrics pkg:SDK Related to an SDK package

Comments

@MrAlias
Copy link
Contributor

MrAlias commented Apr 19, 2022

Blocked by #2799

  • Build the unimplemented structure of the sdk/metric/view package.
  • Ensure all implementation TODOs have an issue tracking them.
@MrAlias MrAlias added pkg:SDK Related to an SDK package area:metrics Part of OpenTelemetry Metrics labels Apr 19, 2022
@MrAlias MrAlias added this to the Metric SDK: Alpha milestone Apr 19, 2022
@MrAlias
Copy link
Contributor Author

MrAlias commented Apr 19, 2022

The current views package in the new_sdk/example branch has the following types:

type (
	View struct {
		cfg Config
	}

	Config struct {
		...
	}
)

I would propose we rename the package to view and just export a Config type. That way func WithViews(views ...view.Config) Option in the sdk/metric reads without a stutter. Additionally, if we then have New(...Option) Config in the view package the call would be WithViews(view.New(...), view.New(...) and this would match our existing configuration patterns.

@MrAlias MrAlias self-assigned this Apr 20, 2022
@MrAlias MrAlias linked a pull request Apr 20, 2022 that will close this issue
@MrAlias
Copy link
Contributor Author

MrAlias commented Apr 21, 2022

I'm wondering if all this view configuration structure is needed. The view is ultimately only going to be passed to a MeterProvider as an option. Why not define it as a type directly in the sdk/metric package with exported fields for all configuration options and an associated NewView function that returns a View with all the defaults set?

@Aneurysm9
Copy link
Member

We use functional options for this sort of configuration pretty much everywhere else. It would feel incongruous to not do so here.

@MrAlias
Copy link
Contributor Author

MrAlias commented May 2, 2022

Closed by #2838

@MrAlias MrAlias closed this as completed May 2, 2022
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 pkg:SDK Related to an SDK package
Projects
No open projects
Development

Successfully merging a pull request may close this issue.

2 participants