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.

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: Refactor Go architecture #43

Merged
merged 12 commits into from
Jul 19, 2023
Merged

refactor: Refactor Go architecture #43

merged 12 commits into from
Jul 19, 2023

Conversation

bhelx
Copy link
Contributor

@bhelx bhelx commented Jul 18, 2023

Refactoring Go SDK to be closer to the API and internal representation that we have with the Rust SDK.

Refactoring Go SDK to be closer to the API and internal representation
that we have with the Rust SDK. Just doing Datadog at the moment and
seeking some feedback then i'll port the others and add docs and tests.
go/adapter.go Outdated
return fmt.Sprintf("%032x", t)
return AdapterBase{
// TODO set to some kind of max, add dump logic
TraceEvents: make(chan TraceEvent, 100),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

My plan is for the adapter to hold a bucket of these events and dump them after MAX is reached or after some timeout occurs. So it may make sense to default this to MAX. but it could also be something configurable by the end programmer.

go/adapter.go Outdated Show resolved Hide resolved
go/bin/datadog/main.go Outdated Show resolved Hide resolved
go/trace_ctx.go Outdated Show resolved Hide resolved
go/telemetry.go Show resolved Hide resolved
@bhelx bhelx marked this pull request as ready for review July 19, 2023 17:53
Collectors: map[*Collector]chan bool{},
return AdapterBase{
// TODO set to some kind of max, add dump logic
TraceEvents: make(chan TraceEvent, 100),
Copy link
Member

Choose a reason for hiding this comment

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

Sends to this channel will block once the channel reaches 100 events. I think thats probably ok, but wondering if this is intentional. We can omit the buffered size and let the channel grow as needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah i think non-buffered might be the way to go but let me think about it for a moment. my intention with using a buffered channel is if the adapter is busy then it may take some time to read off this channel which would block the sender right? By default sending a message to a channel blocks until the receiver synchronizes. Ideally the adapter just pulls messages off this channel and only puts them in a local slice, then it submits them offline in another goroutine. but i haven't done that yet. so this is acting at the bucket.

Copy link
Member

Choose a reason for hiding this comment

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

I see -- yea that makes sense!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll make a follow up for that. Goal will be to add the bucket on the adapter side and test that this channel can't get blocked. We just need to make sure that the adapter's main routine only reads off this channel as fast as possible. And if it can't put the events in a bucket it should just throw them away and log an error. But never block.

Copy link
Member

Choose a reason for hiding this comment

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

SGTM!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#44

go/adapter.go Outdated Show resolved Hide resolved
Copy link
Member

@nilslice nilslice left a comment

Choose a reason for hiding this comment

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

@bhelx - if you wanna give it one last look, and then merge when you're ready I think it's good to go

@bhelx bhelx merged commit c21807b into main Jul 19, 2023
2 checks passed
@bhelx bhelx deleted the go-rearchitecture branch July 19, 2023 23:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants