-
Notifications
You must be signed in to change notification settings - Fork 41
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
Add annotations for opentelemetry-sdk
#189
base: main
Are you sure you want to change the base?
Conversation
# typed: true | ||
|
||
module OpenTelemetry | ||
sig { returns(OpenTelemetry::Trace::TracerProvider) } |
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 can see some internal usages of the SDK, I'm not familiar with OpenTelemetry but isn't it a better experience to return implementation types? For example OpenTelemetry::SDK::Trace::TracerProvider
?
Example call site: OpenTelemetry.tracer_provider.shutdown
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.
Sure, I can do that. Would you also like me to update all the references below to be OpenTelemetry::SDK::
instead of OpenTelemetry::
?
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 so. Looks like API and SDK are 2 separate gems. This annotation is for the SDK given the name so I think it's fine to type things for the SDK specifically.
sig { params(end_timestamp: Integer).void } | ||
def finish(end_timestamp: nil); end |
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.
If the default value is nil
, I think the end_timestamp
should be T.nilable(Integer)
, no?
Type of Change
Changes