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

OpenTelemetry Inner Spans/Manual Telemetry #12

Merged
merged 9 commits into from
Jan 18, 2024
Merged

OpenTelemetry Inner Spans/Manual Telemetry #12

merged 9 commits into from
Jan 18, 2024

Conversation

rjawesome
Copy link
Contributor

@tokebe
Copy link
Member

tokebe commented Jan 8, 2024

@rjawesome This is a great start but something seems off...testing on my local it appears that context isn't properly being carried, so various spans end up having no or incorrect parents when viewed in Jaeger.

@rjawesome
Copy link
Contributor Author

Seems to be working on my end for sync query... If you could let me know some more details about how you are testing then I can try to reproduce it.
image

Another note, JaegerExporter is being deprecated soon and it seems like the library wants people to use HTTP instead of UDP?
image

@tokebe
Copy link
Member

tokebe commented Jan 9, 2024

It may be possible to switch back from JaegerExporter, will need some testing against CI that I can do later. For now, could you tell me your test setup to see if I can replicate the behavior you're seeing?

@rjawesome
Copy link
Contributor Author

rjawesome commented Jan 9, 2024

environment variables: JAEGER_SERVER=localhost
jaeger server: docker run -p 16686:16686 -p 4318:4318 -p 6831:6831/udp -p 6832:6832/udp --restart=no --label='desktop.docker.io/wsl-distro=Ubuntu-16.04' --runtime=runc -d jaegertracing/all-in-one:latest

bte run: pnpm run start
(it should say 5 times that opentelemetry init started before querying)
I'm querying /v1/query

@tokebe
Copy link
Member

tokebe commented Jan 10, 2024

I wasn't able to get any reports to show up in jaeger with your setup. Trying the following setup did work for me (after changing JaegerExporter back to OTLPTraceExporter), but only received traces from biothings-explorer, not biothings-explorer-thread:

Amend package.json > scripts > watch to the following: "nodemon -e js,mjs,json,ts --watch './packages/**/src/*' './packages/**/data/*' --exec 'pnpm run build && node --require ./packages/bte-server/built/controllers/opentelemetry.js .'"

docker run -d --name jaeger -p 16686:16686 -p 6832:6832/udp -p 4318:4318 -p 6381:6831/udp --runtime=runc jaegertracing/all-in-one:latest

JAEGER_HOST=localhost pnpm start

I ran a basic /v1/query as a test.

@rjawesome
Copy link
Contributor Author

Using your setup, adding a timeout seemed to make it more reliable for me (sometimes for me it doesn't work on the first query, so try running it a couple times). Also the HTTP exporter seems more reliable than the Proto for OTLP (see the second code thing)

In taskHandler.js (should require opentelemetry.js at the top as well which is in this branch)

...
 try {
    transaction.finish();
    span.end();
    Telemetry.removeOtelSpan();
  } catch (error) {
    debug("Sentry/OpenTelemetry transaction finish error. This does not affect execution.");
    debug(error);
  }

  await new Promise((resolve, reject) => {
    setTimeout(resolve, 5000);
  })

  debug(`Worker thread ${threadId} completed ${workerData.queue} task.`);
  
...

also, my implementation of trace exporter in opentelemetry.js (seems to be more reliable with HTTP -> see the import)

const { OTLPTraceExporter } = require("@opentelemetry/exporter-trace-otlp-http");
  traceExporter: new OTLPTraceExporter({
    url: `http://${process.env.JAEGER_HOST}:4318/v1/traces`,
  }),

@tokebe tokebe merged commit a3e4a91 into main Jan 18, 2024
0 of 2 checks passed
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