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

metric API is to verbose #2653

Closed
bogdandrutu opened this issue Mar 4, 2022 · 10 comments
Closed

metric API is to verbose #2653

bogdandrutu opened this issue Mar 4, 2022 · 10 comments
Assignees
Labels
area:metrics Part of OpenTelemetry Metrics enhancement New feature or request pkg:API Related to an API package

Comments

@bogdandrutu
Copy link
Member

It is very unintuitive for external users, and too much code is needed and jump around between packages to register a simple Counter. See example:

counter, err := meter.SyncFloat64().Counter("an_important_metric", instrument.WithDescription("Measures the cumulative epicness of the app"))

How can we simplify this to become more intuitive? I would like this to be something like:

counter, err := meter.Float64Counter("an_important_metric", instrument.WithDescription("Measures the cumulative epicness of the app"))

OR

counter, err := meter.Float64Counter("an_important_metric", metric.WithDescription("Measures the cumulative epicness of the app"))
@bogdandrutu bogdandrutu added the enhancement New feature or request label Mar 4, 2022
@Aneurysm9
Copy link
Member

We've discussed this here and here and made the decision to use the package structure reflected in #2587. I don't think it is worth going back to the bike shed on this.

@bogdandrutu
Copy link
Member Author

The discussion #2557 (review) does not seem to be "decided". The PR was closed while the discussion was happening (see the close event happened after few hours since last comment from @XSAM)... But probably this is how the SIG operates.

@alolita
Copy link
Member

alolita commented Mar 4, 2022

@bogdandrutu come join the SIG - Thursdays at 1300 PT - would be good to have you join the discussions.

@bogdandrutu
Copy link
Member Author

bogdandrutu commented Mar 4, 2022

I think feedback from users, especially because I will be one of the first users of this in the collector, does not need to go to the SIG meeting. I can join in 2w when probably this will be 1.0 :)) or people will say is too late to change

@jmacd
Copy link
Contributor

jmacd commented Mar 4, 2022

Could we help the user by the addition of new package-level methods in otel/metric, such as

type Instruments struct {
	Meter
}

func (i Instruments) Float64Counter(name string, opts ...instrument.Option) (syncfloat64.Counter, error) {
	return i.SyncFloat64().Counter(name, opts...)
}

with 11 more functions like the above?

@hanyuancheung hanyuancheung added the area:metrics Part of OpenTelemetry Metrics label Mar 5, 2022
@MadVikingGod
Copy link
Contributor

I am sorry for closing the PR without responding to the last comments. On reflection, I can see how that can look like trying to stifle discussion, which I was not trying to do.

I think, to keep the rest of the project moving, let's try to keep the discussion of further changes to the API here.

@MrAlias
Copy link
Contributor

MrAlias commented Mar 12, 2022

From the SIG meeting last week:

  • The changes proposed here move the API closer to the what we moved away from. There was a desire to move away from that API as it was too bloated and flat. Much of the discussion was synchronous in SIG meetings, but it is summed up in the already linked issue. The SIG meeting discussions were also covering the topics of user feedback we already received for the old API.
  • If there is a desire to submit another proposal for an API it would need to be constructed and submitted soon. We are holding up the release of the API as an experimental package on resolving this. We do not want to do that indefinitely. We have set a deadline of this next Thursday (2022-03-17) to either have a way to address this issue or move on and make the release.

@bogdandrutu
Copy link
Member Author

The changes proposed here move the API closer to the what we moved away from. There was a desire to move away from that API as it was too bloated and flat. Much of the discussion was synchronous in SIG meetings, but it is summed up in the already linked #2526. The SIG meeting discussions were also covering the topics of user feedback we already received for the old API.

I see that issue referenced multiple times, which is a great issue documenting multiple possibilities, but there is no analysis on why that version was picked vs the flatten one.

If there is a desire to submit another proposal for an API it would need to be constructed and submitted soon. We are holding up the release of the API as an experimental package on resolving this. We do not want to do that indefinitely. We have set a deadline of this next Thursday (2022-03-17) to either have a way to address this issue or move on and make the release.

The argument that if you think something is better do the work does not work with me :). I am giving feedback, and if you choose to ignore it is up to you, but this API smells as is, because I have to do an extra "random" call to a func SyncFloat64() which does not make any sense and is not similar with anything I've seen before... As simple as that,

@MrAlias MrAlias assigned MrAlias and unassigned bogdandrutu Mar 17, 2022
@MrAlias
Copy link
Contributor

MrAlias commented Mar 18, 2022

The changes proposed here move the API closer to the what we moved away from. There was a desire to move away from that API as it was too bloated and flat. Much of the discussion was synchronous in SIG meetings, but it is summed up in the already linked #2526. The SIG meeting discussions were also covering the topics of user feedback we already received for the old API.

I see that issue referenced multiple times, which is a great issue documenting multiple possibilities, but there is no analysis on why that version was picked vs the flatten one.

If there is a desire to submit another proposal for an API it would need to be constructed and submitted soon. We are holding up the release of the API as an experimental package on resolving this. We do not want to do that indefinitely. We have set a deadline of this next Thursday (2022-03-17) to either have a way to address this issue or move on and make the release.

The argument that if you think something is better do the work does not work with me :). I am giving feedback, and if you choose to ignore it is up to you, but this API smells as is, because I have to do an extra "random" call to a func SyncFloat64() which does not make any sense and is not similar with anything I've seen before... As simple as that,

Thanks for your feedback! We discussed this proposed change again yesterday in the SIG meeting and came to the decision that we do not plan to pursue this proposal.

An important take-away also from the meeting yesterday was that we plan to do a better job going forward in explaining and documenting the design choices we have explored and evaluated. We recognize that would have helped clarify our new API design better.

@MrAlias MrAlias closed this as completed Mar 18, 2022
@tsloughter
Copy link
Member

I'm not the intended target of this library -- I don't actively develop with Go -- but I felt I should add my experience reading the API spec while investigating the existing implementations in the various languages. Basically I found the Go library confusing because it diverted from the API spec. I know the spec isn't, and can't be, exact since languages differ greatly, but felt it shouldn't differ unless required for idiomatic code.

.net, python, erlang all have create_counter and create_observable_counter functions or methods, while Go is meter.SyncInt64().Counter and meter.AsyncInt64().Counter -- when I would have expected meter.CreateInt64Counter and meter.CreateInt64ObservableCounter.

I don't have a suggestion or know if this is an actual pain other developers would ever feel, just adding a datapoint in case it helps :)

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:API Related to an API package
Projects
No open projects
Status: Done
Development

Successfully merging a pull request may close this issue.

8 participants