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.

Sign up for GitHub

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

Jump to bottom

May be better to return a pointer to View, since it is a pretty large struct #3199

Closed
bogdandrutu opened this issue Sep 20, 2022 · 5 comments
Closed

May be better to return a pointer to View, since it is a pretty large struct #3199

bogdandrutu opened this issue Sep 20, 2022 · 5 comments
Assignees
@jyz0309
Labels
area:metrics Part of OpenTelemetry Metrics enhancement New feature or request pkg:SDK Related to an SDK package

Comments

@bogdandrutu
Copy link
Member

bogdandrutu commented Sep 20, 2022 •
edited

https://github.com/open-telemetry/opentelemetry-go/blob/main/sdk/metric/view/view.go#L54, also change methods to be on the pointer instead of value, to avoid unnecessary copy every time match is called.
@bogdandrutu bogdandrutu added the enhancement New feature or request label Sep 20, 2022
@MrAlias MrAlias added this to the Metric SDK: Beta milestone Sep 20, 2022
@MrAlias MrAlias added pkg:SDK Related to an SDK package area:metrics Part of OpenTelemetry Metrics labels Sep 20, 2022
@jyz0309
Copy link

jyz0309 commented Sep 21, 2022

Can I take this issue? :)

@jyz0309 jyz0309 mentioned this issue Sep 22, 2022
Closed
@bogdandrutu
Copy link
Member Author

bogdandrutu commented Oct 17, 2022 •
edited

The PR was closed based on wrong assumptions:
The benchmark to measure New is not the relevant part, calling funcs on View is the one that worries more if "performance" was seen as the major reason.

I still believe is the correct way to not pollute the stack and every larger object to be heap allocated and avoid unnecessary copies of the struct.

@MrAlias MrAlias removed this from the Metric SDK: Beta milestone Oct 20, 2022
@bogdandrutu
Copy link
Member Author

bogdandrutu commented Oct 25, 2022 •
edited

@MrAlias why did you remove this from the milestone, it is still needed in my opinion...

@jmacd since you commented on that PR, please prove me wrong :)

@MrAlias
Copy link
Contributor

MrAlias commented Oct 25, 2022

@MrAlias why did you remove this from the milestone, it is still needed in my opinion...

We closed that milestone in favor of tracking beta with a project and releases with milestones. It's still on the list of things to do 🙂

@jmacd jmacd mentioned this issue Oct 25, 2022
Merged
@MrAlias MrAlias mentioned this issue Oct 27, 2022
Closed
4 tasks
@MrAlias
Copy link
Contributor

MrAlias commented Dec 9, 2022

The view design was changed in the completion of #3399.

This issue is no longer relevant to the new design. Closing.

@MrAlias MrAlias closed this as completed Dec 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@jyz0309 jyz0309

Labels
area:metrics Part of OpenTelemetry Metrics enhancement New feature or request pkg:SDK Related to an SDK package
Projects
No open projects
Go: Metric SDK (Beta)
Status: Done
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Return pointer to View func jyz0309/opentelemetry-go
3 participants
@bogdandrutu @MrAlias @jyz0309