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

Use logfmt for search tag input format #147

Merged
merged 2 commits into from
Dec 22, 2017

Conversation

tiffon
Copy link
Member

@tiffon tiffon commented Dec 21, 2017

Fixes #145.

Fixes #11.

Changes the input format for tag matching in the search for to logfmt. Now populates a tags query string parameter when submitting the search HTTP request. The tags GET param is JSON with all values converted to strings (per this comment).

This change is backward compatible with URLs that have the former format. When the old format (key:value|key2:value2) is in the URL, it is still parsed but added to the form in the logfmt format.

Added a tooltip that links to information on logfmt.

screen shot 2017-12-21 at 3 41 28 am

Signed-off-by: Joe Farro <joef@uber.com>
@codecov
Copy link

codecov bot commented Dec 21, 2017

Codecov Report

Merging #147 into master will decrease coverage by 0.27%.
The diff coverage is 75.86%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #147      +/-   ##
==========================================
- Coverage   93.04%   92.76%   -0.28%     
==========================================
  Files          85       85              
  Lines        1854     1881      +27     
  Branches      359      366       +7     
==========================================
+ Hits         1725     1745      +20     
- Misses        119      124       +5     
- Partials       10       12       +2
Impacted Files Coverage Δ
src/components/SearchTracePage/TraceSearchForm.js 86.51% <75.86%> (-5.42%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c15a3a2...8d8c058. Read the comment docs.

@@ -128,6 +140,7 @@ export function TraceSearchFormImpl(props) {
const selectedServicePayload = services.find(s => s.name === selectedService);
const operationsForService = (selectedServicePayload && selectedServicePayload.operations) || [];
const noSelectedService = selectedService === '-' || !selectedService;
// const tagInfoIcon = <i className="info circle icon" />;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dead code?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, thanks, will fix.

Values should be in the{' '}
<a href="https://brandur.org/logfmt" rel="noopener noreferrer" target="_blank">
logfmt
</a>{' '}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

perhaps we could give an example, save people a click

e.g. db.statement="select * from User". Use space for conjunctions: error=true http.status_code:503

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The placeholder has two examples. But, I can add a few to the info tooltip, including one that uses quotes.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nevermind, I see your note now. I'll add that to the tooltip 👍

@tiffon
Copy link
Member Author

tiffon commented Dec 21, 2017

There is one more thing I want to change before this is merged: how tag names without values in the old format are handled.

Signed-off-by: Joe Farro <joef@uber.com>
@tiffon
Copy link
Member Author

tiffon commented Dec 22, 2017

@yurishkuro Updated tooltip. Slight variation from your suggestion. Look ok?

screen shot 2017-12-21 at 10 24 17 pm

@yurishkuro
Copy link
Member

great!

@tiffon tiffon merged commit b95786c into master Dec 22, 2017
@yurishkuro yurishkuro deleted the issue-145-logfmt-tag-search-input branch January 29, 2020 15:06
vvvprabhakar pushed a commit to vvvprabhakar/jaeger-ui that referenced this pull request Jul 5, 2021
…fmt-tag-search-input

Use logfmt for search tag input format
Signed-off-by: vvvprabhakar <vvvprabhakar@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants