search

luca-giardina / lara-zipkin

Laravel wrapper for openzipkin/zipkin
6 stars 0 forks source link

Use request uri template for low cardinality on span name. #1

Closed jcchavezs closed 5 years ago

jcchavezs commented 5 years ago

Currently, the span naming is leading to high cardinality by using the requestURI. Instead you should try to use something with low cardinality and predictable if possible. E.g. instead of using /things/123456 you might want to use /things/{thing_id}.

https://github.com/luca-giardina/lara-zipkin/blob/master/src/Middleware/LaraZipkinMiddleware.php#L25

luca-giardina commented 5 years ago

Could you be more specific please? What you think it could be a good improvement into the code for the main span instead of using the requested URI? a random code? Remember that should be just the main span, inside you could use child spans I'm going to improve the readme file :)

jcchavezs commented 5 years ago

By making span name high cardinality (i.e. unlimited number of possible names) what happens is that it becomes hard (near to impossible) to index those spans. Talking specifically about zipkin, when opening the UI you will see that there is a list of span names and that list will be endless with high cardinality.

image

Usual practice is that you use something which is meaningful and predictable (e.g. route with wildcards as /users/{user_id} because all request matching that route will match there.

lfaoro commented 5 years ago

@luca-giardina what's the ETD for this issue please?

luca-giardina commented 5 years ago

with Laravel is very easy to do: Route::current()->uri() I will do some changes switching to a route middleware because it's impossible to use a general middleware and have the laravel router ready. Lumen needs to be tested with later versions because with 5.2.9 I can't get what I need. I will do it asap.

jcchavezs commented 5 years ago

Make sure this does not affect performance. If you have the opportunity of trying this with real traffic would be very good.

We had to do some tricks in symfony and cache the route list because of performance.

Den fre. 8. mar. 2019, 00:17 skrev luca-giardina notifications@github.com:

with Laravel is very easy to do: Route::current()->uri() I will do some changes switching to a route middleware because it's impossible to use a general middleware and have the laravel router ready. Lumen needs to be tested with later versions because with 5.2.9 I can't get what I need. I will do it asap.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/luca-giardina/lara-zipkin/issues/1#issuecomment-470740096, or mute the thread https://github.com/notifications/unsubscribe-auth/AC7sAtenP5jYw0pHRcXDVVXuhuutJtFrks5vUZ3IgaJpZM4bZJBT .

jcchavezs commented 5 years ago

Great, just a suggestion: if no pattern matched for route usually you mark it with not_found or the http verb worst case.

Den lør. 9. mar. 2019, 20:13 skrev luca-giardina notifications@github.com:

Closed #1 https://github.com/luca-giardina/lara-zipkin/issues/1 via 71e7633 https://github.com/luca-giardina/lara-zipkin/commit/71e7633993535385601e1eac03171e7de2b342b7 .

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/luca-giardina/lara-zipkin/issues/1#event-2191952595, or mute the thread https://github.com/notifications/unsubscribe-auth/AC7sAjVYTzqQDGRyfAuBx0D74Hq6XkHuks5vVAe9gaJpZM4bZJBT .