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.

Sign up for GitHub

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
Merged

Use logfmt for search tag input format #147

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ee40410
Use logfmt for search tag input format
tiffon Dec 21, 2017
8d8c058
Better logfmt hint and conversion of tag queries
tiffon Dec 22, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 7 additions & 6 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,12 +5,12 @@
"license": "MIT",
"proxy": {
"/api": {
"target": "http://localhost:16686",
"logLevel": "silent",
"secure": false,
"changeOrigin": true,
"ws": true,
"xfwd": true
"target": "http://localhost:16686",
"logLevel": "silent",
"secure": false,
"changeOrigin": true,
"ws": true,
"xfwd": true
}
},
"homepage": null,
Expand Down Expand Up @@ -54,6 +54,7 @@
"jest": "^21.2.1",
"json-markup": "^1.1.0",
"lodash": "^4.17.4",
"logfmt": "^1.2.0",
"moment": "^2.18.1",
"prop-types": "^15.5.10",
"query-string": "^5.0.0",
Expand Down
87 changes: 76 additions & 11 deletions src/components/SearchTracePage/TraceSearchForm.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,12 +13,15 @@
// limitations under the License.

import React from 'react';
import logfmtParser from 'logfmt/lib/logfmt_parser';
import { stringify as logfmtStringify } from 'logfmt/lib/stringify';
import moment from 'moment';
import PropTypes from 'prop-types';
import queryString from 'query-string';
import { connect } from 'react-redux';
import { bindActionCreators } from 'redux';
import { Field, reduxForm, formValueSelector } from 'redux-form';
import { Popup } from 'semantic-ui-react';
import store from 'store';

import SearchDropdownInput from './SearchDropdownInput';
Expand All @@ -34,11 +37,20 @@ export function getUnixTimeStampInMSFromForm({ startDate, startDateTime, endDate
};
}

export function tagsToQuery(tags) {
export function convTagsLogfmt(tags) {
if (!tags) {
return null;
}
return tags.split('|');
const data = logfmtParser.parse(tags);
Object.keys(data).forEach(key => {
const value = data[key];
// make sure all values are strings
// https://github.com/jaegertracing/jaeger/issues/550#issuecomment-352850811
if (typeof value !== 'string') {
data[key] = String(value);
}
});
return JSON.stringify(data);
}

export function traceIDsToQuery(traceIDs) {
Expand Down Expand Up @@ -117,7 +129,7 @@ export function submitForm(fields, searchTraces) {
lookback,
start,
end,
tag: tagsToQuery(tags) || undefined,
tags: convTagsLogfmt(tags) || undefined,
minDuration: minDuration || null,
maxDuration: maxDuration || null,
});
Expand All @@ -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.

return (
<div className="search-form">
<form className="ui form" onSubmit={handleSubmit}>
Expand All @@ -154,14 +167,24 @@ export function TraceSearchFormImpl(props) {
)}

<div className="search-form--tags field">
<label htmlFor="tags">Tags</label>
<div className="ui input">
<Field
name="tags"
type="text"
component="input"
placeholder="http.status_code:400|http.status_code:200"
<label htmlFor="tags">
Tags{' '}
<Popup
on="click"
trigger={<i className="info circle icon grey" />}
content={
<span>
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 👍

format
</span>
}
/>
</label>
<div className="ui input">
<Field name="tags" type="text" component="input" placeholder="http.status_code=200 error=true" />
</div>
</div>

Expand Down Expand Up @@ -270,6 +293,7 @@ export function mapStateToProps(state) {
end,
operation,
tag: tagParams,
tags: logfmtTags,
maxDuration,
minDuration,
lookback,
Expand Down Expand Up @@ -307,8 +331,49 @@ export function mapStateToProps(state) {
} = convertQueryParamsToFormDates({ start, end });

let tags;
// continue to parse tagParams to remain backward compatible with older URLs
// but, parse to logfmt format instead of the former "key:value|k2:v2"
if (tagParams) {
tags = tagParams instanceof Array ? tagParams.join('|') : tagParams;
// eslint-disable-next-line no-inner-declarations
function convFormerTag(accum, value) {
const parts = value.split(':', 2);
const key = parts[0];
if (key) {
// eslint-disable-next-line no-param-reassign
accum[key] = parts[1] == null ? 'null' : parts[1];
return true;
}
return false;
}

let data;
if (Array.isArray(tagParams)) {
data = tagParams.reduce((accum, str) => {
convFormerTag(accum, str);
return accum;
}, {});
} else if (typeof tagParams === 'string') {
const target = {};
data = convFormerTag(target, tagParams) ? target : null;
}
if (data) {
try {
tags = logfmtStringify(data);
} catch (_) {
tags = 'Parse Error';
}
} else {
tags = 'Parse Error';
}
}
if (logfmtTags) {
let data;
try {
data = JSON.parse(logfmtTags);
tags = logfmtStringify(data);
} catch (_) {
tags = 'Parse Error';
}
}
let traceIDs;
if (traceIDParams) {
Expand Down
122 changes: 79 additions & 43 deletions src/components/SearchTracePage/TraceSearchForm.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,10 +23,10 @@ import store from 'store';

import {
convertQueryParamsToFormDates,
convTagsLogfmt,
getUnixTimeStampInMSFromForm,
mapStateToProps,
submitForm,
tagsToQuery,
traceIDsToQuery,
TraceSearchFormImpl as TraceSearchForm,
} from './TraceSearchForm';
Expand Down Expand Up @@ -93,10 +93,26 @@ describe('conversion utils', () => {
});
});

describe('tagsToQuery()', () => {
it('splits on "|"', () => {
const strs = ['a', 'b', 'c'];
expect(tagsToQuery(strs.join('|'))).toEqual(strs);
describe('convTagsLogfmt()', () => {
it('converts logfmt formatted string to JSON', () => {
const input = 'http.status_code=404 span.kind=client key="with a long value"';
const target = JSON.stringify({
'http.status_code': '404',
'span.kind': 'client',
key: 'with a long value',
});
expect(convTagsLogfmt(input)).toBe(target);
});

// https://github.com/jaegertracing/jaeger/issues/550#issuecomment-352850811
it('converts all values to strings', () => {
const input = 'aBoolKey error=true num=9';
const target = JSON.stringify({
aBoolKey: 'true',
error: 'true',
num: '9',
});
expect(convTagsLogfmt(input)).toBe(target);
});
});

Expand All @@ -110,7 +126,7 @@ describe('conversion utils', () => {

describe('submitForm()', () => {
const LOOKBACK_VALUE = 2;
const LOOKBACK_UNIT = 'd';
const LOOKBACK_UNIT = 's';
let searchTraces;
let fields;

Expand All @@ -121,7 +137,6 @@ describe('submitForm()', () => {
operation: 'op-a',
resultsLimit: 20,
service: 'svc-a',
tags: 'a|b',
};
});

Expand All @@ -142,7 +157,7 @@ describe('submitForm()', () => {
const { start, end } = calls[0][0];
const diffMs = (Number(end) - Number(start)) / 1000;
const duration = moment.duration(diffMs);
expect(duration.asDays()).toBe(LOOKBACK_VALUE);
expect(duration.asSeconds()).toBe(LOOKBACK_VALUE);
});

it('processes form dates when `lookback` is "custom"', () => {
Expand All @@ -165,7 +180,7 @@ describe('submitForm()', () => {
});
});

describe('`fields.tag`', () => {
describe('`fields.tags`', () => {
it('is ignored when `fields.tags` is falsy', () => {
fields.tags = undefined;
submitForm(fields, searchTraces);
Expand All @@ -175,14 +190,19 @@ describe('submitForm()', () => {
expect(tag).toBe(undefined);
});

it('is parsed `fields.tags` is truthy', () => {
const tagStrs = ['a', 'b'];
fields.tags = tagStrs.join('|');
it('is parsed when `fields.tags` is truthy', () => {
const input = 'http.status_code=404 span.kind=client key="with a long value"';
const target = JSON.stringify({
'http.status_code': '404',
'span.kind': 'client',
key: 'with a long value',
});
fields.tags = input;
submitForm(fields, searchTraces);
const { calls } = searchTraces.mock;
expect(calls.length).toBe(1);
const { tag } = calls[0][0];
expect(tag).toEqual(tagStrs);
const { tags } = calls[0][0];
expect(tags).toEqual(target);
});
});

Expand Down Expand Up @@ -271,36 +291,52 @@ describe('mapStateToProps()', () => {
store.get = oldStoreGet;
});

it('derives values from `state.router.location.search` when available', () => {
const { date: startSrc, dateStr: startDate, dateTimeStr: startDateTime } = makeDateParams(-1);
const { date: endSrc, dateStr: endDate, dateTimeStr: endDateTime } = makeDateParams(0);
const common = {
lookback: '2h',
maxDuration: null,
minDuration: null,
operation: 'Driver::findNearest',
service: 'driver',
};
const params = {
...common,
end: `${endSrc.valueOf()}000`,
limit: '999',
start: `${startSrc.valueOf()}000`,
tag: ['error:true', 'span.kind:client'],
};
const expected = {
...common,
endDate,
endDateTime,
startDate,
startDateTime,
resultsLimit: params.limit,
tags: params.tag.join('|'),
traceIDs: null,
};
describe('deriving values from `state.router.location.search`', () => {
let params;
let expected;

state.router.location.search = queryString.stringify(params);
expect(mapStateToProps(state).initialValues).toEqual(expected);
beforeEach(() => {
const { date: startSrc, dateStr: startDate, dateTimeStr: startDateTime } = makeDateParams(-1);
const { date: endSrc, dateStr: endDate, dateTimeStr: endDateTime } = makeDateParams(0);
const tagsJSON = '{"error":"true","span.kind":"client"}';
const tagsLogfmt = 'error=true span.kind=client';
const common = {
lookback: '2h',
maxDuration: null,
minDuration: null,
operation: 'Driver::findNearest',
service: 'driver',
};
params = {
...common,
end: `${endSrc.valueOf()}000`,
limit: '999',
start: `${startSrc.valueOf()}000`,
tags: tagsJSON,
};
expected = {
...common,
endDate,
endDateTime,
startDate,
startDateTime,
resultsLimit: params.limit,
tags: tagsLogfmt,
traceIDs: null,
};
});

it('derives values when available', () => {
state.router.location.search = queryString.stringify(params);
expect(mapStateToProps(state).initialValues).toEqual(expected);
});

it('parses `tag` values in the former format to logfmt', () => {
delete params.tags;
params.tag = ['error:true', 'span.kind:client'];
state.router.location.search = queryString.stringify(params);
expect(mapStateToProps(state).initialValues).toEqual(expected);
});
});

it('fallsback to default values', () => {
Expand Down
Loading