-
Notifications
You must be signed in to change notification settings - Fork 224
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
[OPIK-558] Typescript SDK - basic client #919
base: main
Are you sure you want to change the base?
Conversation
…led one (ESM vs CJS)
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've been focus on the auto-generation set up in this review. My only question is about the .gitignore
file.
I recommend splitting this PR in two:
- Just all the Fern set up and changes in the auto-generated classes.
- Then all your typescript custom code for the SDK, including examples etc.
Reasons:
- You get a proper review on the SDK code, which is the actual valuable thing. Now it's lost in the middle of all the auto-generated changes.
- We're following the convention of pushing auto-generated code in isolation. Among other reasons, you're code can be stale. It's actually, due to some latest changes in the backend.
} | ||
"organization": "Opik", | ||
"version": "0.45.3" | ||
} |
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.
Very picky and minor: keep the extra end of line.
"version": "0.45.0" | ||
} | ||
"organization": "Opik", | ||
"version": "0.45.3" |
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.
Minor: if you're going to update the Fern version, I'd go with the last one, which is 0.46.5
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.
Yes, I updated it weeks ago when I had to due a bugfix that Fern has and now they released a more updated version so agree, I'll keep it updated to the latest one, thanks for noticing
sdks/typescript/.gitignore
Outdated
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 it's better to maintain just the .gitignore
file in the root folder of the repo.
@andrescrz this is a first draft to have a first iteration of the TS SDK, it's not final, it's something that'll be changed in next iterations so no point to review the API in the SDK yet I'll split the PRs as suggested but I don't think we should hold the TS SDK changes too long, again, we'll iterate in the actual API soon, now we need a first iteration of a build and publish an alpha version in NPM, that counts about the "examples" you are asking, it's not needed right now since we'll change a lot of things, when it's stable, we'll add more proper examples than the one it's now FYI, the code is not outdated, I ran the In any case, we can merge on my way back, thanks for the feedback |
const projectName = spanData.projectName ?? this.opik.config.projectName; | ||
const spanWithId: SavedSpan = { | ||
id: uuid(), | ||
startTime: new Date(), |
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 consider having utc
for all dates probably
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 would also add a way to get the time from arguments and if it is not specified, i would it the current one
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.
Well, the spread operator acts like that:
{
<default values>,
...argumentValues,
<forced values>
}
In this case id
and startTime
have default values and projectName
and traceId
are forced values
I think the structure of it is correct but I could get the defaults in another way more centralized if that's what you mean :)
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.
yeah, sorry, misread it
.findProjects({ name: projectName }) | ||
.asRaw(); | ||
|
||
const project = projectsPage.content?.find( |
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 have the endpoint to get a project by name api/v1/private/projects/retrieve
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 call :)
workspaceName: this.config.workspaceName, | ||
}); | ||
} | ||
public loadProject = async (projectName: string): Promise<ProjectPublic> => { |
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 don't how much we want to align the typescript and the python sdks. i noticed that on the sdk side there is an approach of having get_or_create_...
for such cases
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.
For now this is just internal use, I'll align more the things in the near future not at this stage
const projectName = trace.projectName ?? this.config.projectName; | ||
const traceWithId: SavedTrace = { | ||
id: uuid(), | ||
startTime: new Date(), |
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 would add a way to customize a startTime
. as we do in python
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.
@aadereiko as mentioned above, it's customizable, you can add it in the trace({ startTime: ... })
call:
interface TraceData extends Omit<ITrace, "startTime"> {
startTime?: Date;
}
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 made it optional since we can set a default here (now), same for id
Details
openapi.yaml
to add global headers (Authorization / Comet-Workspace) to the global Fern config (affected current Python SDK generated files, if it's not good for it I can split into different fern configurations, fern does not support custom overrides for each group)Issues
Resolves #
Testing
Documentation