-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Refactor the SDK helpers, create MeterImpl #560
Conversation
Reviewers: see how I intend to build off of this functionality in #566 |
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.
Did a quick review, will need to make it more thorough tomorrow. But overall looks good. It certainly brings some nice simplifications and reduces the repetitive code.
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 need to dig deeper into this, but wanted to provide my initial feedback. It is mostly small naming/docs questions. But, one overall thing I'm wondering is if it was considered to use Async*
/Sync*
instead of Asynchronous*
/Synchronous*
>
I want to add that this PR is far from perfect. One of the decisions that I had trouble making was whether the new This matters for the code in As it stands, therefore, the code in This means that the code in #566 doesn't tell the whole story. The real SDK can acquire uniqueness checking by being wrapped in a new MeterImpl as shown in that PR, but the global package has to expand the access checks into each constructor by hand. So part of me wonders how it would look if: (1) all SDKs were required to implement |
I added some detail in #566. I now think it won't actually help to assume that all |
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 at a place it should be merged.
That said it does look like there can be some subsequent cleanup, like you mentioned. In particular, I think the sdkhelpers.go
file could use being split up and probably some more documentation, but looks good for a first iteration.
The only real concern I've included in the GetDescriptor
method on the SDK. It seems like something we should decide on removing/keeping sooner rather than later.
Overall, excellent work. I think this paired with what it enables in the uniqueness checking to follow it is a step in the correct direction.
I added a Synchronous->Sync and Asynchronous->Async change. I generally agree, @MrAlias, that a bunch of cleanups and moving things into different files would help. I tried to keep things in place to help the diff be readable. I'd be all for a change that only moves things around for clarity without any functional changes. |
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.
Looks good. All I found were documentation nits.
Co-Authored-By: Krzesimir Nowak <[email protected]>
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.
Nice work!
One minor non-blocking comment.
This reduces the boilerplate needed to implement the
Meter
.api/metric/sdkhelpers.go
now defines a 4-methodMeterImpl
interface which supports general-purpose constructors instead of specific ones, i.e.,NewSynchronousInstrument
andNewAsynchronousInstrument
. The two implementations (the Mock and the real SDK) are simplified. The global Meter implementation is also refactored to share as much of possible of this code, however it is written not to require the use ofMeterImpl
, as it is just a helper. (An SDK could decide to implement the constructors directly.)This refactoring is a precursor to #514, since after we have the four-method
MeterImpl
a name-uniqueness wrapper will simply wrap around theMeterImpl
and provide uniqueness checking.Removes the
Unregister()
method for observers, since it was not in the spec. This can be restored in another PR if needed, but I would prefer we ask users to shut down the SDK instead of unregistering observers.