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

[feature request] JS example with HttpInstrumentation #1061

Closed
codefromthecrypt opened this issue Oct 14, 2024 · 6 comments · Fixed by #1121
Closed

[feature request] JS example with HttpInstrumentation #1061

codefromthecrypt opened this issue Oct 14, 2024 · 6 comments · Fixed by #1121
Labels
enhancement New feature or request language: js Related to JavaScript or Typescript integration

Comments

@codefromthecrypt
Copy link

Is your feature request related to a problem? Please describe.

I have an local example which uses OpenAI and HttpInstrumentation, but the http spans for the OpenAI calls seem to be in separate traces.

Describe the solution you'd like

I'd like an example which registers the normal HttpInstrumentation and is verified to create sub-spans, not new traces.

Describe alternatives you've considered

Just not using HttpInstrumentation

Additional context

@codefromthecrypt codefromthecrypt added enhancement New feature or request triage Issues that require triage labels Oct 14, 2024
@github-project-automation github-project-automation bot moved this to 📘 Todo in phoenix Oct 14, 2024
@dosubot dosubot bot added the language: js Related to JavaScript or Typescript integration label Oct 14, 2024
@mikeldking
Copy link
Contributor

Hey @codefromthecrypt we have a few examples in this repo that does this (e.x.

tracer.startActiveSpan("chat", async (span) => {
)

In general the best documentation is probably just opentelemetry https://opentelemetry.io/docs/languages/js/

@codefromthecrypt
Copy link
Author

codefromthecrypt commented Oct 14, 2024

ok, so mainly I have found some instrumentation set spans active in openai such that instrumentation (such as http) will end up the child without explicit instrumentation.

something like...

        /** @type {import('openai/core').APIPromise} */
        const apiPromise = context.with(
          spanContext,
          () => original.apply(this, args)
        )

...

        // Non-streaming.
        apiPromise
          .then(
            /**
             * https://platform.openai.com/docs/api-reference/chat/object
             * @param {import('openai').OpenAI.ChatCompletion} result
             */
            (result) => {

So, while we can wrap with manual instrumentation, it seems ideal to have implicit instrumentation like above or similar, since not everyone will want to add code to their apps. Is this something interesting here, or should we go with the manual tracing approach?

@mikeldking mikeldking removed the triage Issues that require triage label Oct 15, 2024
@mikeldking
Copy link
Contributor

@codefromthecrypt I generally agree with your sentiment that people would prefer not to have to add code. However I think with promises you would not expect the spans to nest under each other in your example above

// Context here is A
const promiseA = new Promise(async (resolve, reject) => {
    // Perform some action
    try {
    // Span for fetch is created here under context A  
    // Let's call this span X
    const data = await fetch();
    } catch(e) {
      reject(e)
    }
})

promiseA.then((result) => {
   // Context here is also A, a sibling of X under A
})

the .then() chaining thus only creates siblings (which in this case ends up being on their own)

I hope I understood your problem correctly.

@codefromthecrypt
Copy link
Author

I can understand how there can be different opinions on instrumentation.

Generally, http spans are a child of the thing that creates them, which allows to walk up the tree. This is the usual causal implementation. As peers, especially in a fan-out of requests, there's no way to associate cause directly (you can by correlation, but that's not causal). I'm not intimate with otel semantics, or if correlated http spans are normal pattern or not. It wouldn't be in zipkin for sure, but again that's not otel.

Regardless, I think you understand the issue, and I'm happy to close this out. On the original request (provide an example), you went there and beyond. Much obliged and see you around!

@Parker-Stafford
Copy link
Contributor

Parker-Stafford commented Nov 14, 2024

Just following up here, support for this (for openai instrumentation at least) is coming in #1121

@Parker-Stafford
Copy link
Contributor

This has been fixed and released in version 1.1.0 of the OpenAIInstrumentor cc - @codefromthecrypt

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request language: js Related to JavaScript or Typescript integration
Projects
None yet
3 participants