feat: base opentelemetry integration - #363
Conversation
mcollina
left a comment
There was a problem hiding this comment.
I would generically prefer if we provided hooks (similar to fastify, or even simpler) for APMs to plug into. This is totally ok but it's non-scalable as there are a few different implementations for this.
As stated in #346 (comment) this is just a cleanup of the previous PR with the tracer declaration and the base mercurius span. Unfortunately, I don't have time to push this implementation further for now. But, I'm not sure to understand what you mean by providing hooks, as opentelemetry is already providing an API for the end-user to extend the tracing and how it is plugged. This implementation is based on opentelemetry recommendations: https://open-telemetry.github.io/opentelemetry-js/#library-authors |
Some APMs do not implement OpenTelemetry but they have similar concepts/libraries. They'd need to put logic in the same places. Instead of calling opentracing directly, we could do: app.graphql.setTracing({
... // all the methods for those points
}) |
|
I also support hooks, I am trying to build an SDK for graphmetrics.io and we are not openmetrics compliant for now (we will consider it when the spec stabilizes a bit). |
|
@Sytten this is probably not the right issue to talk about. Maybe you can open a new one? Or just send a PR. |
|
FWIW I think the aspirations of opentelemetry is to avoid having different APM products all have their own interfaces, and instead give library authors one generally accepted API to integrate against. We'd build integrations against |
| "@graphql-tools/merge": "^6.2.4", | ||
| "@graphql-tools/schema": "^6.2.4", | ||
| "@graphql-tools/utils": "^6.2.4", | ||
| "@opentelemetry/api": "^0.13.0", |
There was a problem hiding this comment.
@opentelemetry/api version 0.16.0 changes the API significantly, if we do want to get this in we should try to build against the most up to date tracer API and active-context-getting functions they have available (the getSpan, setSpan stuff)
| // this query errored | ||
| const err = new MER_ERR_GQL_VALIDATION() | ||
| err.errors = cachedError.validationErrors | ||
| span.end() |
There was a problem hiding this comment.
Usually on erros error info is added to span as status code
const { SpanStatusCode } = require('@opentelemetry/api')
span.setStatus({
code: SpanStatusCode.ERROR,
message: error.message,
});
As well as OK status on success.
span.setStatus({
code: SpanStatusCode.OK,
});
Also to avoid multiple span.end() I prefer following pattern:
const result = tracer.startActiveSpan(
`<span_name>`,
{ attributes: <spanAttributes> },
async (span) => {
try {
// do something
span.setStatus({ code: SpanStatusCode.OK });
// return if needed
} catch (error) {
span.setStatus({
code: SpanStatusCode.ERROR,
message: error.message,
});
throw error;
} finally {
span.end();
}
},
);
return result;But it might be little bit less performant. And this one is available in most recent opentelemetry API, not sure about version from package.json.
This PR adds a base mercurius span to the fastifyGraphql decorator and the cached attribute.