Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[API Epic 1/4] Metric api prototype #2557
[API Epic 1/4] Metric api prototype #2557
Changes from 1 commit
281a662
7bd4f00
754d83c
b2dab0c
a92285c
4dab081
ad95b08
16a42f5
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is not a good package name for the functionality. It does not reflect any "metric" functionality. @rakyll would like your input here, since I know you have good input for these cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bogdan, do you think splitting/organizing packages into type based units like here is a problem? Even though the package names don't represent what's in them, I don't have a good suggestion. asyncfloat64instrument would be too verbose and putting all instruments in the same package might be too big. I think the choice here is a fair compromise if we need to split instruments into multiple packages. Disclaimer: I didn't have much time to think to come up with a better structure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In golang the type name is not very important since majority of the times you don't use that in the assignments or declaration. That is one of the main reason I believe that the split in the package base on the input type does not make sense.
I think something like 'instrument.Float64Counter' and 'instrument.Float64ObserverCounter' is better because of that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bogdandrutu There are 12 instruments in total, (int64, float64) x (6 instruments). This prototype has them in 4 packages, 3 instruments each. How many packages would you propose having?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I created a change that explored this approach. Here.
There are pros and cons to both approaches. There is also a takeaway that I will be incorporating into this patch.
The pros for grouping everything into the
instrument
package is that when looking for the kinds of instruments they are all documented together. This is a nice feature if we can have minimal other things in the package.The downside of that approach is that the names become very unwieldy.
What is now a
Becomes a
The other pro is that we could get rid of the namespacing we do, the Meter Interface would then become the 12 Create functions + register. This was discussed in #2526, and this API in the PR is made this way because of that discussion.
The one takeaway I have is that the nonrecording implementations can be extracted into a separate package. That I will incorporate regardless of this discussion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personally I prefer the one package approach, and that package can either be "metric" where Meter and MeterProvider are defined.
I also believe that "Sync" can be removed from the names to shorter the common case:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I slightly prefer the one package approach. End-users can find all
instrument
s in the same place. When the generics feature is ready, we can have intuitive naming for generics functions without creating a new package:I have no opinion for Async/Observable naming.