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

Simpler usage of Observable instruments #1715

Merged

Conversation

cijothomas
Copy link
Member

@cijothomas cijothomas commented May 6, 2024

There are 2 ways today to provide callbacks for Observable Instruments today:

  1. As part of instrument creation itself (via with_callback)
  2. After instrument creation, via meter.register_callback

After careful consideration, I'm leaning towards removing option 2. While it offers flexibility by allowing one callback to be associated with multiple instruments, and the ability to "un-register", it significantly increases complexity and requires more extensive testing to ensure correctness. It also has a lot of public API exposure, so removing this from 1st stable release also allow us to maintain a manageable scope.

In the upcoming PR(s), I'll make changes to modify all examples/tests to utilize option 1 (this PR just adds a single test) If there are no scenarios where option 2 is essential, I'll proceed with its removal in subsequent iterations.

@open-telemetry/rust-approvers Please share your thoughts on removing option2. (To be clear, this is proposed to be removed to keep scope in control, as this is not only complex to use, but equally challenging to ensure sufficient test coverage/correctness)

Edit: Updated all examples and tests to use option1. (There is dire shortage of tests overall :( )

@cijothomas cijothomas requested a review from a team May 6, 2024 23:32
@lalitb
Copy link
Member

lalitb commented May 7, 2024

It also has a lot of public API exposure, so removing this from 1st stable release also allow us to maintain a manageable scope.

Should we remove it from the code, or move it under the experimental flag?

@cijothomas
Copy link
Member Author

It also has a lot of public API exposure, so removing this from 1st stable release also allow us to maintain a manageable scope.

Should we remove it from the code, or move it under the experimental flag?

I'd prefer to remove it from code completely. Putting under experimental flag without investing in tests is not really helpful. Also, with feature flags, we still need to make sure things build successfully, somewhat defeating the goal of reducing scope!

Not all languages have implemented this capability, and it is not a MUST in the spec either. Let us start with offering just the simple API.

Copy link
Contributor

@TommyCpp TommyCpp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should be cautious on removing APIs we advertised in examples. Even to control the scope. If we remove too many of those APIs people may not want to upgrade to a stable version. Or they will take the oppounity to switch to something more stable

TommyCpp

This comment was marked as duplicate.

@cijothomas
Copy link
Member Author

I think we should be cautious on removing APIs we advertised in examples. Even to control the scope. If we remove too many of those APIs people may not want to upgrade to a stable version. Or they will take the oppounity to switch to something more stable

Yes, I completely understand your concern! It is a delicate balance. Delaying the stable release indefinitely also poses its own drawbacks. I am hesitant to keep any API without thoroughly considering its use cases and adequate test coverage, which sometimes conflicts with preserving backward compatibility and keeping initial scope in check. Given the planned beta release is closer, now is the apt moment to accept breaking API changes while we're still in the alpha phase.

@lalitb
Copy link
Member

lalitb commented May 7, 2024

Can we instead document "option 2" as unstable, and clearly state that it is not part of the beta release. Additionally, also mark it as deprecated. This way, it won't block us for beta release, and would be heads up to users that this MAY be removed before the stable release. Just concerned about removing it abruptly even in current alpha state :)

@cijothomas
Copy link
Member Author

Just concerned about removing it abruptly even in current alpha state :)

I'll be happy to discuss more in the community call, but if we cannot break public API in an alpha release, what is the whole point of the notion of "alpha"?

@TommyCpp
Copy link
Contributor

TommyCpp commented May 7, 2024

but if we cannot break public API in an alpha release, what is the whole point of the notion of "alpha"?

We can and probably should break public API in alpha release. And I am OK with removing advance API like views. But this API seems like a fundational one and one we have been advertising(using example) a lot. I personally think we should have it by the time we GA the metrics signal. Thus, marking it as unstable/experiemental during beta phase seems most ideal in this case.

Either case given we won't have it stable we should still change the examples

@cijothomas
Copy link
Member Author

But this API seems like a fundational one and one we have been advertising(using example) a lot

We are not losing functionality, just a diff. way to do the same. The examples are modified to show the alternate option (which, btw, already existed, but examples didn't show it well)

@cijothomas
Copy link
Member Author

I personally think we should have it by the time we GA the metrics signal.

Any particular reason why we need that? We have other (simpler) ways to register callbacks already!

Copy link
Member

@lalitb lalitb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM for these examples/tests changes.

@TommyCpp TommyCpp merged commit 845f2f0 into open-telemetry:main May 8, 2024
18 checks passed
@cijothomas cijothomas deleted the cijothomas/metric-observables1 branch May 8, 2024 15:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants