search

Vinelab / tracing-laravel

Distributed tracing (OpenTracing) for Laravel made easy
MIT License
77 stars 21 forks source link

Consider using idiomatic zipkin tags #6

Open jcchavezs opened 4 years ago

jcchavezs commented 4 years ago

Currently there are a couple of things that don't follow idiomatic naming in zipkin.

First, we usually use VERB /path/for/route as span names instead of a fixed name as it is less useful when it comes to search for an specific operation. E.g. GET /images/{image_id}. Notice the usage of {image_id} instead of the actual ID as high cardinality on this can make your span search struggle.

Second, there are a bunch of idiomatic zipkin tags that I would recommend to use instead of custom ones in https://github.com/Vinelab/tracing-laravel/blob/master/src/Middleware/TraceRequests.php#L75. See: https://github.com/openzipkin/zipkin-php/blob/master/src/Zipkin/Tags.php

FInally I am concerned about the automatic adding request_headers into the tags, sensitive information like JWT can be carried here and at most you want ti to be optional and a concious decision because it implies some privacy issues. Also not sure https://github.com/Vinelab/tracing-laravel/blob/master/src/Middleware/TraceRequests.php#L80 includes query parameters, they also can carry sensitive information like api_key.

adiachenko commented 4 years ago

Thank you for the feedback, there is a lot of good suggestions here. I made plans to include these in future versions of the package.

adiachenko commented 4 years ago

Hello @jcchavezs, the latest v0.7.0 release of the package incorporates some of the improvements that you kindly suggested including better default for HTTP span names and configuration options for logged headers.

That being said, I'll be withholding a transition to idiomatic Zipkin tags for now. It seems like OpenTracing semantic guide doesn't have a particularly varied selection of idiomatic tags and I'd rather not mix and match between these and our own custom ones unless there is good reason for it.

One such example is an error tag that we avail off in the latest release and helps with visually identifying spans for operations that couldn't be completed successfully. I am all for adopting names like these that have real practical benefits when working with query UI. Otherwise, I'd rather preserve consistency across all implementations that happen to already consume this package which affects us internally at Vinelab also.

I'll keep this discussion open for reference and in case we discover other uses for idiomatic tags that are worth exploring.

steveoliver commented 1 year ago

Another use case: Idiomatic tags like span.kind seem to be required in order to take advantage of things like Grafana Tempo's Service Graphs (https://grafana.com/docs/tempo/latest/metrics-generator/service_graphs).