Flow complete fix by andreibratu · Pull Request #9 · humanloop/humanloop-node

andreibratu · 2025-01-06T16:24:54Z

Simplified the code that kept track of the trace built by decorators - instead of relying on OT's context, we are only peeking at the status of the ancestor on the level above -
The exporter correctly completes flow traces - instead of waiting for the program exit to upload the spans, I am attaching a new span attribute instructing the exporter which spans' logs should be uploaded before completing the flow log. To make this work, the processor keeps a running list of spans as they start, which belong in a particular trace. The list is attached to the flow span during the onEnd signal. The spans created by utilities will no longer have a UUIDv4 name but be named after the log type they carry, e.g., humanloop.prompt or humanloop.flow. This is also more dogmatic with how OT does things

harry-humanloop · 2025-01-07T10:38:19Z

src/otel/exporter.ts

        await this.shutdown();
    }

+    private completeFlowLog(spanId: string) {


this needs more comments and a docstring.
this is doing a lot - mutating this.prerequisite's values, conditionally doing many things (the delete and the update). hard to follow/check if there are bugs.

harry-humanloop · 2025-01-07T10:40:23Z

src/otel/helpers.ts

- */
-export function generateSpanId(): string {
-    return uuidv4();
+    return span.name.startsWith("humanloop.");


should be a const

harry-humanloop · 2025-01-07T10:41:03Z

src/otel/processor.ts

    onStart(span: Span, _: Context): void {
+        const spanId = span.spanContext().spanId;
+        const parentSpanId = span.parentSpanId;
+        if (span.name === "humanloop.flow") {


"humanloop.flow" should be a const

harry-humanloop · 2025-01-07T10:41:13Z

src/otel/processor.ts

-                // All children/ instrumentor spans have arrived, we can process the
+                // All instrumentor spans have arrived, we can process the
                // Humanloop parent span owning them
+                if (span.name === "humanloop.flow") {


should be a const

harry-humanloop · 2025-01-07T10:57:18Z

src/utilities/flow.ts

        const flowMetadataKey = createContextKey(HUMANLOOP_TRACE_FLOW_CTX_KEY);
        // @ts-ignore
-        return opentelemetryTracer.startActiveSpan(generateSpanId(), async (span) => {
+        return opentelemetryTracer.startActiveSpan("humanloop.flow", async (span) => {


do these not need to be unique?

(should be a const)

I've realized it's not necessary. The name is supposed to hint at the type of span you're looking at, which I'm leveraging in the processor-exporter to complete the trace.

https://opentelemetry.io/docs/specs/semconv/http/http-spans/#name

harry-humanloop · 2025-01-07T10:58:50Z

src/utilities/prompt.ts


        // @ts-ignore
-        return opentelemetryTracer.startActiveSpan(generateSpanId(), async (span) => {
+        return opentelemetryTracer.startActiveSpan("humanloop.prompt", async (span) => {


humanloop.prompt should be a const

harry-humanloop · 2025-01-07T10:59:10Z

src/utilities/prompt.ts

+                output = await func(inputs, messages);
+            } catch (err: any) {
+                console.error(`Error calling ${func.name}:`, err);
+                // @ts-ignore


what's the tsignore doing?

seems to be a leftover, removed

olehvell-h · 2025-01-07T18:40:06Z

src/otel/exporter.ts

+     *
+     * @param spanId - The ID of the span that has been uploaded.
+     */
+    private notifySpanUploaded(spanId: string) {


noob question - why notifySpanUploaded and not e.g. markSpanCompleted. Is notify OT jargon?

fair point will change - no OT jargon, just bad taste on my side

olehvell-h · 2025-01-07T18:47:02Z

src/otel/processor.ts

+        if (span.name === HUMANLOOP_FLOW_SPAN_NAME) {
+            this.prerequisites.set(spanId, []);
+        }
+        if (parentSpanId !== undefined && isHumanloopSpan(span)) {


noob question - (parentSpanId !== undefined && isHumanloopSpan(span)) this is to check if it's a span from flow child ?

yep - i am building a flat list of all logs found inside a trace. this list is passed as an attribute on the flow log span to the expoter. when they're all uploaded, the flow log is also marked as complete

olehvell-h

Looks good... to the extent of my OT knowledge 😅

Side point: We as a team (and I personally), need a bit of learning on OT. It takes mental energy to understand OT-specific jargon and details before focusing on code-related matters.

Thank you for writing PR description and " Please assign yourself as a reviewer if you have the time." - like this bit

Fix flow complete bug

22f6fd3

olehvell-h self-requested a review January 7, 2025 09:18

harry-humanloop reviewed Jan 7, 2025

View reviewed changes

PR feedback

4658307

olehvell-h reviewed Jan 7, 2025

View reviewed changes

renamed exporter span uploaded function

8feeed5

olehvell-h reviewed Jan 7, 2025

View reviewed changes

olehvell-h approved these changes Jan 7, 2025

View reviewed changes

andreibratu merged commit f0bf780 into master Jan 9, 2025
3 checks passed

Conversation

andreibratu commented Jan 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

olehvell-h left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

andreibratu commented Jan 6, 2025 •

edited

Loading