autotelic / fastify-opentelemetry

A Fastify plugin that utilizes the OpenTelemetry API to provide request tracing.
MIT License
72 stars 12 forks source link

Use low cardinality span name #40

Closed cvrajeesh closed 3 years ago

cvrajeesh commented 3 years ago

otel spec recommending to use low cardinality span names

HTTP spans MUST follow the overall guidelines for span names. Many REST APIs encode parameters into URI path, e.g. /api/users/123 where 123 is a user id, which creates high cardinality value space not suitable for span names. In case of HTTP servers, these endpoints are often mapped by the server frameworks to more concise HTTP routes, e.g. /api/users/{user_id}, which are recommended as the low cardinality span names

Do you think it's a good choice to routerPath from fastify request instead of raw request URL by default when generating span name? https://github.com/autotelic/fastify-opentelemetry/blob/dd3a6b64f45da36288b331e09f1f681f3160e842/index.js#L13-L15

HW13 commented 3 years ago

Thanks for bringing this to our attention @cvrajeesh! 👍

I think using routerPath is the way to go. But we'll need to update opts.formatSpanName to receive the entire Fastify request object.

https://github.com/autotelic/fastify-opentelemetry/blob/dd3a6b64f45da36288b331e09f1f681f3160e842/index.js#L99

Technically this will be a breaking change, although it looks like most fields in request.raw exist top-level in request.

Would you like to open a PR to implement this?

cvrajeesh commented 3 years ago

sure @HW13, I'll try to find some time this week and submit a PR.

Technically this will be a breaking change, although it looks like most fields in request.raw exist top-level in request.

for TypeScript users this will be a breaking change as the formatSpanName signature is going to change https://github.com/autotelic/fastify-opentelemetry/blob/dd3a6b64f45da36288b331e09f1f681f3160e842/fastify-opentelemetry.d.ts#L27