zipkin-ruby icon indicating copy to clipboard operation
zipkin-ruby copied to clipboard
verified publisher icon openzipkin

Annotate http.url instead of http.path

Open jcarres-mdsol opened this issue 6 years ago • 6 comments

I do not care about the host but I want the query parameters and could not find another standard annotation better than url. In many cases query parameters are key to understand why a particular request is slow. For instance if you control pagination via query parameters you may be requesting a huge amount of data. Or if you do some complex filtering on your query params you may be requesting a very complex set that requires lots of processing.

@ykitamura-mdsol @adriancole

jcarres-mdsol avatar May 20 '19 22:05 jcarres-mdsol

One of our services is referring path info so this might be a breaking change for others too. How about keeping path as is and adding query with the value of QUERY_STRING in the Rack Env?

ykitamura-mdsol avatar May 20 '19 23:05 ykitamura-mdsol

We are all over the place but I think other traces tend to do http.url also so our other service may have trouble already

jcarres-mdsol avatar May 21 '19 00:05 jcarres-mdsol

@ykitamura-mdsol I'm going to do both url and path with a deprecation on path. Eventually to be removed

jcarres-mdsol avatar May 21 '19 03:05 jcarres-mdsol

Honestly I would discourage you to do this change:

  1. Query parameters are usually not annotated for privacy issues. People could include private keys or access tokens into that and that is not desired.
  2. Annotating the full URL just because of the query parameters sound to me something extreme. If there is a need for do such thing I would instead add an option for passing a whitelist of query parameters you might want to record under exceptional conditions and as a conscious decision to record them.

Does any of the things I said make sense?

tir. 21. mai 2019, 00:11 skrev Jordi Polo Carres [email protected]:

I do not care about the host but I want the query parameters and could not find another standard annotation better than url. In many cases query parameters are key to understand why a particular request is slow. For instance if you control pagination via query parameters you may be requesting a huge amount of data. Or if you do some complex filtering on your query params you may be requesting a very complex set that requires lots of processing.

@ykitamura-mdsol https://github.com/ykitamura-mdsol @adriancole https://github.com/adriancole

You can view, comment on, or merge this pull request online at:

Commit Summary

  • Annotate http.url instead of http.path

File Changes

Patch Links:

  • https://github.com/openzipkin/zipkin-ruby/pull/154.patch
  • https://github.com/openzipkin/zipkin-ruby/pull/154.diff

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/openzipkin/zipkin-ruby/pull/154?email_source=notifications&email_token=AAXOYASGMHLB7FM2DCPEMATPWMOXJA5CNFSM4HOFZ7JKYY3PNVWWK3TUL52HS4DFUVEXG43VMWVGG33NNVSW45C7NFSM4GU2QACQ, or mute the thread https://github.com/notifications/unsubscribe-auth/AAXOYAXUT5SGG6S7WMVF5WTPWMOXJANCNFSM4HOFZ7JA .

jcchavezs avatar May 21 '19 13:05 jcchavezs

I could always add URL behind a configuration option and default it to not annotate it. In fact, thinking about this, I prefer that approach.

To me is crazy to have private info in query params but I guess the case does exist.

jcarres-mdsol avatar May 22 '19 00:05 jcarres-mdsol

Yeah, old APIs use apitoken being passed in the query parameters.

In the other hand I like the idea is this being a optional feature at client middleware level. What I am not sure is if the http.path should be dropped when this option is enabled as path became more a defacto annotation among libraries lately.

ons. 22. mai 2019, 02:35 skrev Jordi Polo Carres [email protected]:

I could always add URL behind a configuration option and default it to not annotate it. In fact, thinking about this, I prefer that approach.

To me is crazy to have private info in query params but I guess the case does exist.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/openzipkin/zipkin-ruby/pull/154?email_source=notifications&email_token=AAXOYAQJEXWMH2JMTOBDJLTPWSIL3A5CNFSM4HOFZ7JKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODV5R7GY#issuecomment-494608283, or mute the thread https://github.com/notifications/unsubscribe-auth/AAXOYARNN75NAH2OLAC4IRTPWSIL3ANCNFSM4HOFZ7JA .

jcchavezs avatar May 24 '19 08:05 jcchavezs