-
Notifications
You must be signed in to change notification settings - Fork 3
feat(telementry): add streaming methods #571
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
base: main
Are you sure you want to change the base?
Conversation
cfaef0e to
0129aca
Compare
0129aca to
056b6b0
Compare
| import type * as Core from '../../core'; | ||
| import type * as Shared from '../shared'; |
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 we are using verbatimModuleSyntax in the rest of our code but not here, so it seems risky to patch existing code just to make them type-only imports, when that might be part of what stainless needs to auto-resolve in the future when it makes updates
|
|
||
| export declare namespace Traces { | ||
| export { type TraceListResponse as TraceListResponse, type TraceListParams as TraceListParams }; | ||
| export type { TraceListResponse, TraceListParams }; |
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.
Likewise, I don't see why we'd change existing code that doesn't need to be changed here, this is probably going to make automerges harder
|
|
||
| export declare namespace Logs { | ||
| export { type LogListResponse as LogListResponse, type LogListParams as LogListParams }; | ||
| export type { LogListResponse, LogListParams }; |
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.
Probably don't edit this, for the same reason as other comments
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.
Oop, yeah I think the formatter did this :/
Thanks for catching these!
| import type * as Core from '../../core'; | ||
| import type * as Shared from '../shared'; | ||
|
|
||
| export interface BaseListParams { |
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 wonder if we could split out this custom code into a separate file so that the only change to stainless's generated files is something like export * from …
| const newStartTime = new Date(new URL(newLinks.next).searchParams.get('end')!); | ||
| if (newStartTime.getTime() === startTime.getTime()) break; // No new data, stop | ||
|
|
||
| for (const item of newData) { |
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 we'd use yield* here too because newData is iterable
| start: startTime.toISOString(), | ||
| end: endTime.toISOString(), | ||
| ...query, | ||
| } as TParams; |
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.
Somewhat nervous that this doesn't actually do type checking in the hard way - we can't say satisfies TParams here or const listParams: TParams = here because concrete instances can't have generic types. Probably not a big thing but I don't think TypeScript is protecting us that much here.
| query: StreamParams, | ||
| options?: Core.RequestOptions, | ||
| ): AsyncGenerator<TData> { | ||
| const { frequency = 1000 } = query; |
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.
We should probably use a higher default here, 1/s is a pretty fast ping.
Add .stream to logs and traces so you can "watch" them