-
Notifications
You must be signed in to change notification settings - Fork 0
Instrumentation for Lanchain4j's OpenAI Chat Model #24
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
Conversation
8db59a5 to
8c94b9c
Compare
8c94b9c to
f09d73b
Compare
| @SneakyThrows | ||
| void testSyncChatCompletion() { | ||
| // Mock the OpenAI API response | ||
| wireMock.stubFor( |
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.
TODO: my next quality-of-life change will be to switch out stubs for VCR-like replay (wiremock supports this apparently). Some time in the next week or two
delner
left a comment
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.
Some questions but nothing blocking!
| public record Options(String providerName) {} | ||
|
|
||
| @SuppressWarnings("unchecked") | ||
| private static <T> T getPrivateField(Object obj, String fieldName) |
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.
This question is just for my own education: it looks like we're using reflection to access the private fields in order to instrument them, correct? What are the performance/stability risks associated with reflection? Are there other practical alternatives for instrumentation?
In the Ruby world, we generally would avoid accessing private fields because of the potential for instability (e.g. someone in a patch version changes the API.)
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 that's right, we're using reflection. There isn't much risk in this case because we'll just fail to apply instrumentation if something goes wrong
Performance is pretty good with reflection, but even if it wasn't this is only done once during client build
There isn't a viable alternative right now, but once we get into auto instrumentation for java we'll have more options
| Span span = startNewSpan(getSpanName(providerInfo)); | ||
| try (Scope scope = span.makeCurrent()) { | ||
| tagSpan(span, request, providerInfo); | ||
| final long startTime = System.nanoTime(); |
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.
What is nanoTime? Is it wall clock time or is it something else?
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.
Basically wall clock time. It's an increasing nanosecond counter from an arbitrary starting point
| + " after %d attempts", | ||
| minSpanCount, spans.size(), attempts)); | ||
| } | ||
| Thread.sleep(1000); |
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.
Why do you need to wait & sleep? To read off the OTel thread? Is there a faster, more directly way to do this synchronously in the test suite?
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.
Usually waiting isn't needed but some of the streaming tests finish their spans after this method is invoked for the first time
I feel like there should be a better way to do this, but the only gotcha is I'm using the built in otel utils to collect spans so I'm not sure what hooks I would have to insert concurrency signaling stuff
I'm making some other changes to the test harness in another branch. I'll add this to that work. At the very least I can dial down the sleep time (10ms should be plenty)
No description provided.