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

Consider using idiomatic zipkin tags #6

Open
jcchavezs opened this issue Sep 30, 2019 · 3 comments · Fixed by #7
Open

Consider using idiomatic zipkin tags #6

jcchavezs opened this issue Sep 30, 2019 · 3 comments · Fixed by #7
Assignees
Labels
enhancement New feature or request

Comments

@jcchavezs
Copy link
Contributor

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
Copy link
Collaborator

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
Copy link
Collaborator

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.

@adiachenko adiachenko reopened this Oct 24, 2019
@steveoliver
Copy link
Contributor

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).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants