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.

Sign up for GitHub

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

Jump to bottom

feat: Add Observe SDK support #47

Merged
merged 11 commits into from
Aug 21, 2024
Merged

feat: Add Observe SDK support #47

merged 11 commits into from
Aug 21, 2024

Conversation

mhmd-azeez
Copy link
Collaborator

Fixes #21

mhmd-azeez
mhmd-azeez commented Dec 11, 2023
View reviewed changes
extism.go Show resolved Hide resolved
mhmd-azeez
mhmd-azeez commented Dec 11, 2023
View reviewed changes
extism.go Show resolved Hide resolved
mhmd-azeez
mhmd-azeez commented Dec 11, 2023
View reviewed changes
extism.go Show resolved Hide resolved
mhmd-azeez
mhmd-azeez commented Dec 11, 2023
View reviewed changes
extism.go Show resolved Hide resolved
mhmd-azeez
mhmd-azeez commented Dec 11, 2023
View reviewed changes
extism.go Outdated Show resolved Hide resolved
@mhmd-azeez
Copy link
Collaborator Author

@nilslice @bhelx While the Go SDK supports instrument_enter, instrument_exit, and instrument_memory_grow, it doesn't seem to support span_enter and its friends. Should I implement them here, or should I open a PR in the observe-sdk repo?

@nilslice
Copy link
Member

@nilslice @bhelx While the Go SDK supports instrument_enter, instrument_exit, and instrument_memory_grow, it doesn't seem to support span_enter and its friends. Should I implement them here, or should I open a PR in the observe-sdk repo?

Oh yea that's right. That would be a great PR on the observe-sdk repo!

@bhelx
Copy link
Contributor

bhelx commented Dec 11, 2023

I can add those onto the Go SDK today. I think I already have a branch for it.

@mhmd-azeez mhmd-azeez mentioned this pull request Jan 10, 2024
Merged
@bhelx
Copy link
Contributor

bhelx commented Feb 19, 2024

What should we do with this PR? been on my review list for a long time

@nilslice
Copy link
Member

In our meeting last week we decided to let it sit until things progress with migration to wasi-observe. But if we can use it in the meantime, I'd be all for updating to the latest API and merging with some experimental note.

The biggest change in-flux that would possibly impact this is the addition of a Context parameter to the instrumentation functions.

cc/ @chrisdickinson

@bhelx bhelx removed their request for review March 25, 2024 14:12
@zshipko zshipko removed their request for review June 25, 2024 14:26
@mhmd-azeez mhmd-azeez requested a review from G4Vi August 18, 2024 10:38
G4Vi
G4Vi requested changes Aug 19, 2024
View reviewed changes
Copy link
Contributor

@G4Vi G4Vi left a comment

Choose a reason for hiding this comment

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

Overall, looks good, happy to see this land soon.

README.md Outdated Show resolved Hide resolved
go.mod Outdated Show resolved Hide resolved
@mhmd-azeez
Copy link
Collaborator Author

@G4Vi thanks for your feedback, I think the PR is ready to be merged

@mhmd-azeez mhmd-azeez merged commit ebab3e7 into main Aug 21, 2024
3 checks passed
@mhmd-azeez mhmd-azeez deleted the feat/observe-integration branch August 21, 2024 14:47
@clarkmcc clarkmcc mentioned this pull request Oct 5, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nilslice nilslice nilslice left review comments

@G4Vi G4Vi G4Vi approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add Observe SDK support
4 participants
@mhmd-azeez @nilslice @bhelx @G4Vi