zipkin-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From GitBox <...@apache.org>
Subject [GitHub] [incubator-zipkin] adriancole commented on issue #2572: trim whitespace from the annotation query elements
Date Thu, 09 May 2019 06:49:00 GMT
adriancole commented on issue #2572: trim whitespace from the annotation query elements
URL: https://github.com/apache/incubator-zipkin/pull/2572#issuecomment-490766430
 
 
   sounds good
   
   On Thu, May 9, 2019, 2:45 PM Jorg Heymans <notifications@github.com> wrote:
   
   > The query string is parsed into a map keyed with the annotation keys. So in
   > fact the last one specified in the query wins, even though it can be the
   > least specific one. An easy fix would be to only overwrite keys in the map
   > in case of a tag parameter and drop annotation parameters if a tag value is
   > present already. WDYT?
   >
   > Op do 9 mei 2019 02:20 schreef Adrian Cole <notifications@github.com>:
   >
   > > *@adriancole* commented on this pull request.
   > > ------------------------------
   > >
   > > In zipkin/src/test/java/zipkin2/storage/QueryRequestTest.java
   > > <
   > https://github.com/apache/incubator-zipkin/pull/2572#discussion_r282301553
   > >
   > > :
   > >
   > > > + // spaces in http.path mixed with 'and'
   > > + assertThat(queryBuilder.parseAnnotationQuery("fo and o and bar and
   > http.path = /a ").annotationQuery)
   > > + .containsOnly(entry("fo", ""), entry("o", ""), entry("bar", ""),
   > entry("http.path", "/a"));
   > > + // http.path in the beginning, more spaces
   > > + assertThat(queryBuilder.parseAnnotationQuery(" http.path = /a and fo
   > and o and bar").annotationQuery)
   > > + .containsOnly(entry("fo", ""), entry("o", ""), entry("bar", ""),
   > entry("http.path", "/a"));
   > > + // @adriancole said this would be hard to parse, annotation containing
   > spaces
   > > + assertThat(queryBuilder.parseAnnotationQuery("L O L").annotationQuery)
   > > + .containsOnly(entry("L O L", ""));
   > > + // annotation with spaces combined with tag
   > > + assertThat(queryBuilder.parseAnnotationQuery("L O L and http.path =
   > /a").annotationQuery)
   > > + .containsOnly(entry("L O L", ""), entry("http.path", "/a"));
   > > + assertThat(queryBuilder.parseAnnotationQuery("bar =123 and L O L and
   > http.path = /a and A B C").annotationQuery)
   > > + .containsOnly(entry("L O L", ""), entry("http.path", "/a"),
   > entry("bar", "123"), entry("A B C", ""));
   > > + // border case, L O L as a tag and annotation, seems that the
   > annotation takes precedence
   > > + assertThat(queryBuilder.parseAnnotationQuery("L O L=123 and L O L and
   > http.path = /a").annotationQuery)
   > >
   > > the backend will do a redundant query. Technically, the most specific
   > > would win based on the query rules.
   > >
   > > —
   > > You are receiving this because you authored the thread.
   > > Reply to this email directly, view it on GitHub
   > > <
   > https://github.com/apache/incubator-zipkin/pull/2572#discussion_r282301553
   > >,
   > > or mute the thread
   > > <
   > https://github.com/notifications/unsubscribe-auth/AABPKAAYLLSPE43M7AYK2PLPUNU6TANCNFSM4HLTSUWQ
   > >
   > > .
   > >
   >
   > —
   > You are receiving this because you were mentioned.
   > Reply to this email directly, view it on GitHub
   > <https://github.com/apache/incubator-zipkin/pull/2572#issuecomment-490765427>,
   > or mute the thread
   > <https://github.com/notifications/unsubscribe-auth/AAAPVV7D6XZIBMHBG2OVPNTPUPB7NANCNFSM4HLTSUWQ>
   > .
   >
   

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

Mime
View raw message