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

doc(opentelemetry): add tracing documentation with zipkin and opentelemetry #341

Merged
merged 8 commits into from
Nov 13, 2020

Conversation

PacoDu
Copy link
Contributor

@PacoDu PacoDu commented Oct 31, 2020

Waiting for @opentelemetry/instrumentation-graphql release.

@PabloSzx
Copy link
Member

Maybe it's better for open-telemetry.md to be in docs/integrations/

@PacoDu PacoDu linked an issue Oct 31, 2020 that may be closed by this pull request
@PacoDu
Copy link
Contributor Author

PacoDu commented Nov 6, 2020

TODO: switch to @autotelic/fastify-opentelemetry

@mcollina
Copy link
Collaborator

mcollina commented Nov 6, 2020

@adamovittorio PTAL

@adamovittorio
Copy link

@projectjudge FYI, we can try it with the gcp/prometheus exporter and see if we can remove the custom code for the headers propagation in fastify. Then in the ctx we have access to the tracer that we can use to instantiate custom spans.

@jonnydgreen
Copy link
Contributor

@adamovittorio for sure, we can give it a go and update the decorator for our custom spans as well if needed

@PacoDu
Copy link
Contributor Author

PacoDu commented Nov 11, 2020

I switched to @autotelic/fastify-opentelemetry for the documentation.

It's not perfect, here is the trace I get with the configuration described in the documentation:

For the _service { sdl } requested by the gateway from service-add I've a clean trace:
image

But for the example query { add(x: 1, y: 2):
image

@PacoDu
Copy link
Contributor Author

PacoDu commented Nov 11, 2020

Ok ... the gateway doesn't work as expected with a query returning a Scalar, it returns null. Modifying the schema to return an object fixed the issue.
Edit: the bug is related to mercurius-js/mercurius-gateway#23 service-add.js needs to extend the Query otherwise it doesn't work.

Opentelemetry is perfectly working 🎉 :
image

@PacoDu PacoDu marked this pull request as ready for review November 11, 2020 01:54
Copy link
Collaborator

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@mcollina
Copy link
Collaborator

I think we should document that this would not work with graphql-jit. Or does it?

@PacoDu
Copy link
Contributor Author

PacoDu commented Nov 11, 2020

I think we should document that this would not work with graphql-jit. Or does it?

I didn't test this with jit enabled, I suppose it will not work for the graphql trace, but we will still get the http trace at least.
Maybe we can add some mercurius specific spans / attributes in another PR.

@PacoDu
Copy link
Contributor Author

PacoDu commented Nov 11, 2020

Normal:
image

Jitted:
image

Edit: I didn't enable the custom span compute-add in the screenshots above, but it works with the jitted version:
image

Copy link
Collaborator

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@mcollina mcollina merged commit 3a68352 into mercurius-js:master Nov 13, 2020
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.

Feature Request: Tracing support
5 participants