[Go] Simple Collector #1976
Open
[Go] Simple Collector #1976
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
tberman
commented
May 22, 2025
Comment on lines
216
to
218
| let collector = Arc::from_raw(*c as *const Collector); | ||
| let clone = collector.clone(); | ||
| let _ = Arc::into_raw(clone); |
There was a problem hiding this comment.
This can't be real.
There has to be a better way.
The docs implied that Arc::from_raw basically 'consumes' the reference so we need to keep leaking it. ChatGPT agreed. I can't imagine this is right.
There was a problem hiding this comment.
Maybe I could Box an Arc pointer? and then use the box to go across the go/rust boundary. is that real?
There was a problem hiding this comment.
I think it is, and its probably the best way, gonna try it!
There was a problem hiding this comment.
Couldn't quite get it to work, so back to the drawing board (Arc pointers are not copyable for obvious reasons)
There was a problem hiding this comment.
you gotta do from box. will send over the code!
tberman
commented
May 22, 2025
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Changes requested ❌
Reviewed everything up to ca90574 in 1 minute and 56 seconds. Click for details.
- Reviewed
2763lines of code in14files - Skipped
0files when reviewing. - Skipped posting
5draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. integ-tests/go/test.go:14
- Draft comment:
Good use of the new Collector API by instantiating it with NewCollector(). Consider documenting or deferring cleanup if a destroy method is available, to avoid potential memory leaks. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
2. integ-tests/go/test.go:16
- Draft comment:
Passing the collector using WithCollector looks correct. Ensure that the underlying CallFunction properly handles the collectors parameter. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
3. integ-tests/go/test.go:91
- Draft comment:
The loop retrieving call information is thorough; however, note that the variable 'timing' is reused inside the loop (shadowing the outer 'timing'). Consider renaming the inner 'timing' variable for clarity. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% While variable shadowing is generally not ideal, in this case both variables are used in very localized scopes with clear purposes. The outer timing is for the "last" object, while the inner timing is clearly for each "call". The scopes don't overlap in a confusing way. The code is still readable and maintainable as is. Variable shadowing can sometimes lead to bugs and confusion. The suggestion to rename for clarity isn't wrong. However, the shadowing here is in well-separated scopes with clear purposes. The code remains readable and the variable names make sense in their contexts. While the comment identifies real shadowing, this is a minor stylistic issue that doesn't impact code correctness or maintainability enough to warrant a change.
4. integ-tests/go/test.go:141
- Draft comment:
Commented-out code for additional stream tests is acceptable in a test file. Remove if unnecessary or add a comment explaining its purpose. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
5. engine/language_client_go/pkg/collector.go:221
- Draft comment:
Typographical suggestion: The method name 'RawLlmResponse' is inconsistent with the usual uppercase acronym 'LLM'. Consider renaming it to 'RawLLMResponse' for clarity. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% This is a new file being added, so the comment is about changed code. The suggestion is technically correct - there is inconsistency in capitalization. However, this is a relatively minor stylistic issue that doesn't affect functionality. The rules say to only keep comments that clearly require code changes, and to avoid obvious or unimportant suggestions. The inconsistency could cause confusion for developers reading the code, and fixing it would improve code quality. Consistent naming is a good practice. While consistent naming is good, this is a very minor stylistic issue. The meaning is still clear and the function works correctly either way. This feels more like a nitpick than an important code quality issue. The comment should be deleted as it points out a minor stylistic inconsistency that doesn't significantly impact code quality or functionality.
Workflow ID: wflow_qqaZFEpCEZqdyv5s
You can customize 
| return wrap(createUsage, usagePtr), nil | ||
| } | ||
|
|
||
| func (f *FunctionLog) Id() (string, error) { |
There was a problem hiding this comment.
Typographical suggestion: The method name Id is generally written as ID in Go. Consider renaming it to ID for consistency.
There was a problem hiding this comment.
Caution
Changes requested ❌
Reviewed b0a3e20 in 2 minutes and 3 seconds. Click for details.
- Reviewed
257lines of code in5files - Skipped
0files when reviewing. - Skipped posting
3draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. engine/baml-lib/baml-types/src/tracing/events.rs:250
- Draft comment:
Good improvement: returning an empty string for an empty HTTP body avoids unnecessary UTF-8 conversion. Consider adding a brief comment to document this behavior for future maintainers. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
2. engine/language_client_go/pkg/collector.go:145
- Draft comment:
The wrap() function registers a cleanup via AddCleanup. Ensure that this mechanism correctly handles any errors or potential double-free scenarios. A brief note/comment could help clarify its safety contract. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%None
3. integ-tests/go/test.go:24
- Draft comment:
Consider removing the commented-out goroutine block if it's not needed, to keep the test code clean. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
Workflow ID: wflow_6XXh5h5VVebWb1jV
You can customize
| Ok(null()) | ||
| } | ||
| "text" => { | ||
| let text = http_body.text().unwrap(); |
There was a problem hiding this comment.
Avoid using unwrap() on the result of http_body.text(). This FFI boundary should propagate errors instead of panicking on invalid UTF-8 data.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Looking for feedback on the approach. I think it can be macro'd and made better but curious what y'all think.
If it looks good I will finish out the interface and get it ready to actually be merged.
Important
Introduces a collector mechanism in the Go client for the BAML engine, enhancing function call tracking and logging.
call_collector_functiontolib.rsfor invoking collector functions.call_function_from_cinlib.rsto accept collectors.AaaSamOutputFormatinclient.go.j2to use collectors.CallCollectorFunctioninbaml_cffi_wrapper.candbaml_cffi_wrapper.h.CallFunctionFromCinbaml_cffi_wrapper.candbaml_cffi_wrapper.hto include collectors.collector.gofor managing collectors and their interactions.runtime.goto pass collectors toCallFunction.exports.goto handle collector functions.test.goto test collector functionality withAaaSamOutputFormat.This description was created by
for b0a3e20. You can customize this summary. It will automatically update as commits are pushed.