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

Remove ImmutableJS, refactor search selectors #48

Merged
merged 10 commits into from
Aug 8, 2017
2 changes: 0 additions & 2 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,6 @@
"flow-bin": "^0.36.0",
"fuzzy": "^0.1.1",
"global": "^4.3.0",
"immutable": "^3.8.1",
"is-promise": "^2.1.0",
"isomorphic-fetch": "^2.2.1",
"json-markup": "^1.0.0",
Expand All @@ -49,7 +48,6 @@
"react-dom": "^15.5.0",
"react-ga": "^2.1.2",
"react-helmet": "^3.1.0",
"react-immutable-proptypes": "^2.1.0",
"react-metrics": "^2.2.3",
"react-redux": "^4.4.5",
"react-router": "^2.7.0",
Expand Down
23 changes: 10 additions & 13 deletions src/components/DependencyGraph/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,6 @@ export default class DependencyGraphPage extends Component {
render() {
const { nodes, links, error, dependencies, loading } = this.props;
const { graphType } = this.state;
const serviceCalls = dependencies.toJS();
if (loading) {
return (
<div className="m1">
Expand All @@ -86,16 +85,17 @@ export default class DependencyGraphPage extends Component {

const GRAPH_TYPE_OPTIONS = [{ type: 'FORCE_DIRECTED', name: 'Force Directed Graph' }];

if (serviceCalls.length <= 100) {
if (dependencies.length <= 100) {
GRAPH_TYPE_OPTIONS.push({ type: 'DAG', name: 'DAG' });
}
return (
<div className="my2">
<Menu tabular>
{GRAPH_TYPE_OPTIONS.map(option =>
<Menu.Item
name={option.name}
active={graphType === option.type}
key={option.type}
name={option.name}
onClick={() => this.handleGraphTypeChange(option.type)}
/>
)}
Expand All @@ -112,7 +112,7 @@ export default class DependencyGraphPage extends Component {
}}
>
{graphType === 'FORCE_DIRECTED' && <DependencyForceGraph nodes={nodes} links={links} />}
{graphType === 'DAG' && <DAG serviceCalls={serviceCalls} />}
{graphType === 'DAG' && <DAG serviceCalls={dependencies} />}
</div>
</div>
);
Expand All @@ -121,17 +121,14 @@ export default class DependencyGraphPage extends Component {

// export connected component separately
function mapStateToProps(state) {
const dependencies = state.dependencies.get('dependencies');
let nodes;
const { dependencies, error, loading } = state.dependencies;
let links;
if (dependencies && dependencies.size > 0) {
const nodesAndLinks = formatDependenciesAsNodesAndLinks({ dependencies });
nodes = nodesAndLinks.nodes;
links = nodesAndLinks.links;
let nodes;
if (dependencies && dependencies.length > 0) {
const formatted = formatDependenciesAsNodesAndLinks({ dependencies });
links = formatted.links;
nodes = formatted.nodes;
}
const error = state.dependencies.get('error');
const loading = state.dependencies.get('loading');

return { loading, error, nodes, links, dependencies };
}

Expand Down
11 changes: 8 additions & 3 deletions src/components/SearchTracePage/TraceSearchForm.js
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,7 @@ export function TraceSearchFormComponent(props) {
name="service"
component={SearchDropdownInput}
className="ui dropdown"
items={services.concat({ name: '-' }).map(s => ({ text: s.name, value: s.name }))}
items={services.concat({ name: '-' }).map(s => ({ text: s.name, value: s.name, key: s.name }))}
/>
</div>

Expand All @@ -102,7 +102,7 @@ export function TraceSearchFormComponent(props) {
name="operation"
component={SearchDropdownInput}
className="ui dropdown"
items={operationsForService.concat('all').map(op => ({ text: op, value: op }))}
items={operationsForService.concat('all').map(op => ({ text: op, value: op, key: op }))}
/>
</div>}

Expand Down Expand Up @@ -190,7 +190,12 @@ export function TraceSearchFormComponent(props) {
TraceSearchFormComponent.propTypes = {
handleSubmit: PropTypes.func,
submitting: PropTypes.bool,
services: PropTypes.arrayOf(PropTypes.string),
services: PropTypes.arrayOf(
PropTypes.shape({
name: PropTypes.string,
operations: PropTypes.arrayOf(PropTypes.string),
})
),
selectedService: PropTypes.string,
selectedLookback: PropTypes.string,
};
Expand Down
4 changes: 2 additions & 2 deletions src/components/SearchTracePage/TraceSearchResult.js
Original file line number Diff line number Diff line change
Expand Up @@ -55,15 +55,15 @@ export default function TraceSearchResult({ trace, durationPercent = 100 }) {
<span className="trace-search-result--spans">
{numberOfSpans} span{numberOfSpans > 1 && 's'}
</span>
{numberOfErredSpans &&
{Boolean(numberOfErredSpans) &&
<span className="trace-search-result--erred-spans">
{numberOfErredSpans} error{numberOfErredSpans > 1 && 's'}
</span>}
</div>
<div className="col col-6">
{sortBy(services, s => s.name).map(service =>
<div key={service.name} className="inline-block mr1 mb1">
<TraceServiceTag key={service.name} service={service} />
<TraceServiceTag service={service} />
</div>
)}
</div>
Expand Down
2 changes: 1 addition & 1 deletion src/components/SearchTracePage/TraceSearchResult.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ const testTraceProps = {
services: [
{
name: 'Service A',
numberOfApperancesInTrace: 2,
numberOfSpans: 2,
percentOfTrace: 50,
},
],
Expand Down
6 changes: 3 additions & 3 deletions src/components/SearchTracePage/TraceServiceTag.js
Original file line number Diff line number Diff line change
Expand Up @@ -23,17 +23,17 @@ import React from 'react';
import colorGenerator from '../../utils/color-generator';

export default function TraceServiceTag({ service }) {
const { name, numberOfApperancesInTrace } = service;
const { name, numberOfSpans } = service;
return (
<div className="ui mini label" style={{ borderLeft: `5px solid ${colorGenerator.getColorByKey(name)}` }}>
{name} ({numberOfApperancesInTrace})
{name} ({numberOfSpans})
</div>
);
}

TraceServiceTag.propTypes = {
service: PropTypes.shape({
name: PropTypes.string.isRequired,
numberOfApperancesInTrace: PropTypes.number.isRequired,
numberOfSpans: PropTypes.number.isRequired,
}).isRequired,
};
2 changes: 1 addition & 1 deletion src/components/SearchTracePage/TraceServiceTag.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ it('<SearchTracePage /> tests', () => {
<TraceServiceTag
service={{
name: 'Service A',
numberOfApperancesInTrace: 1,
numberOfSpans: 1,
}}
/>
);
Expand Down
46 changes: 19 additions & 27 deletions src/components/SearchTracePage/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -18,29 +18,23 @@
// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
// THE SOFTWARE.

import _values from 'lodash/values';
Copy link
Member

Choose a reason for hiding this comment

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

Is this a common way of naming single lodash functions? Historically, this is generally meant for private variables.

Are we opposed to bring in the whole lib because of size? If so, I suppose we could do this style as long as we are consistent throughout the application.

Copy link
Member Author

Choose a reason for hiding this comment

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

@saminzadeh I have mixed feelings about the name, myself, for the same reason. I went with _values because

  • values would be an odd name for it as it's a function not a collection of values
    • Come to think of it, though, _values has basically the same issue
  • _values is reminiscent of _.values

I noticed another project using the same approach, but I'm definitely open to alternatives... Maybe valuesFn? (or lodashValues? Long names are a bother, though.)

Yes, importing by function (also called cherry-picking, I think) yields smaller builds. Consistency in the project, in this respect, is a WIP.

Copy link
Member

Choose a reason for hiding this comment

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

Ok cool, im open to the lodash function style of _values as an alias for _.values. It would be sweet if we could use this babel plugin but then we'd have to eject. https://github.com/lodash/babel-plugin-lodash

import PropTypes from 'prop-types';
import React, { Component } from 'react';
import { bindActionCreators } from 'redux';
import { connect } from 'react-redux';
import { Field, reduxForm, formValueSelector } from 'redux-form';
import { Link } from 'react-router';
import { Sticky } from 'react-sticky';
import * as jaegerApiActions from '../../actions/jaeger-api';

import JaegerLogo from '../../img/jaeger-logo.svg';

import * as jaegerApiActions from '../../actions/jaeger-api';
import TraceSearchForm from './TraceSearchForm';
import TraceSearchResult from './TraceSearchResult';
import TraceResultsScatterPlot from './TraceResultsScatterPlot';
import {
transformTraceResultsSelector,
getSortedTraceResults,
LONGEST_FIRST,
SHORTEST_FIRST,
MOST_SPANS,
LEAST_SPANS,
MOST_RECENT,
} from '../../selectors/search';
import * as orderBy from '../../model/order-by';
import { sortTraces, getTraceSummaries } from '../../model/search';
import { getPercentageOfDuration } from '../../utils/date';
import getLastXformCacher from '../../utils/get-last-xform-cacher';

Expand All @@ -52,18 +46,18 @@ let TraceResultsFilterForm = () =>
<div className="field inline">
<label htmlFor="traceResultsSortBy">Sort</label>
<Field name="sortBy" id="traceResultsSortBy" className="ui dropdown" component="select">
<option value={MOST_RECENT}>Most Recent</option>
<option value={LONGEST_FIRST}>Longest First</option>
<option value={SHORTEST_FIRST}>Shortest First</option>
<option value={MOST_SPANS}>Most Spans</option>
<option value={LEAST_SPANS}>Least Spans</option>
<option value={orderBy.MOST_RECENT}>Most Recent</option>
<option value={orderBy.LONGEST_FIRST}>Longest First</option>
<option value={orderBy.SHORTEST_FIRST}>Shortest First</option>
<option value={orderBy.MOST_SPANS}>Most Spans</option>
<option value={orderBy.LEAST_SPANS}>Least Spans</option>
</Field>
</div>
</div>;
TraceResultsFilterForm = reduxForm({
form: 'traceResultsFilters',
initialValues: {
sortBy: MOST_RECENT,
sortBy: orderBy.MOST_RECENT,
},
})(TraceResultsFilterForm);
const traceResultsFiltersFormSelector = formValueSelector('traceResultsFilters');
Expand Down Expand Up @@ -194,13 +188,13 @@ SearchTracePage.propTypes = {
};

const stateTraceXformer = getLastXformCacher(stateTrace => {
const { traces: traceMap, loading, error: traceError } = stateTrace.toJS();
const traces = Object.keys(traceMap).map(traceID => traceMap[traceID]);
return { tracesSrc: { traces }, loading, traceError };
const { traces: traceMap, loading, error: traceError } = stateTrace;
const { traces, maxDuration } = getTraceSummaries(_values(traceMap));
return { traces, maxDuration, loading, traceError };
});

const stateServicesXformer = getLastXformCacher(stateServices => {
const { services: serviceList, operationsForService: opsBySvc, error: serviceError } = stateServices.toJS();
const { services: serviceList, operationsForService: opsBySvc, error: serviceError } = stateServices;
const services = serviceList.map(name => ({
name,
operations: opsBySvc[name] || [],
Expand All @@ -211,18 +205,17 @@ const stateServicesXformer = getLastXformCacher(stateServices => {
function mapStateToProps(state) {
const query = state.routing.locationBeforeTransitions.query;
const isHomepage = !Object.keys(query).length;
const { tracesSrc, loading, traceError } = stateTraceXformer(state.trace);
const { traces, maxDuration } = transformTraceResultsSelector(tracesSrc);
const { traces, maxDuration, loading, traceError } = stateTraceXformer(state.trace);
const { services, serviceError } = stateServicesXformer(state.services);
const sortBy = traceResultsFiltersFormSelector(state, 'sortBy');
const traceResultsSorted = getSortedTraceResults(traces, sortBy);
const errorMessage = serviceError || traceError ? `${serviceError || ''} ${traceError || ''}` : '';
const sortBy = traceResultsFiltersFormSelector(state, 'sortBy');
sortTraces(traces, sortBy);

return {
isHomepage,
sortTracesBy: sortBy,
traceResults: traceResultsSorted,
numberOfTraceResults: traceResultsSorted.length,
traceResults: traces,
numberOfTraceResults: traces.length,
maxTraceDuration: maxDuration,
urlQueryParams: query,
services,
Expand All @@ -233,7 +226,6 @@ function mapStateToProps(state) {

function mapDispatchToProps(dispatch) {
const { searchTraces, fetchServices } = bindActionCreators(jaegerApiActions, dispatch);

return {
searchTraces,
fetchServices,
Expand Down
2 changes: 1 addition & 1 deletion src/components/TracePage/TraceTimelineViewer/SpanDetail.js
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ function CollapsePanel(props) {
);
}
CollapsePanel.propTypes = {
header: PropTypes.element.isRequired,
header: PropTypes.node.isRequired,
onToggleOpen: PropTypes.func.isRequired,
children: PropTypes.element.isRequired,
open: PropTypes.bool.isRequired,
Expand Down
11 changes: 3 additions & 8 deletions src/components/TracePage/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -168,18 +168,13 @@ export default class TracePage extends Component {

// export connected component separately
function mapStateToProps(state, ownProps) {
const { params: { id } } = ownProps;

let trace = state.trace.getIn(['traces', id]);
const { id } = ownProps.params;
let trace = state.trace.traces[id];
if (trace && !(trace instanceof Error)) {
trace = trace.toJS();
trace = dropEmptyStartTimeSpans(trace);
trace = hydrateSpansWithProcesses(trace);
}

const loading = state.trace.get('loading');

return { id, loading, trace };
return { id, trace, loading: state.trace.loading };
}

function mapDispatchToProps(dispatch) {
Expand Down
11 changes: 5 additions & 6 deletions src/reducers/index.test.js → src/model/order-by.js
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,8 @@
// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
// THE SOFTWARE.

import jaegerReducers from './index';
import traceReducer from './trace';

it('jaegerReducers should contain the trace reducer', () => {
expect(jaegerReducers.trace).toBe(traceReducer);
});
export const MOST_RECENT = 'MOST_RECENT';
Copy link
Member

Choose a reason for hiding this comment

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

I think this file needs license info, npm run add-license

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks. Done.

export const LONGEST_FIRST = 'LONGEST_FIRST';
export const SHORTEST_FIRST = 'SHORTEST_FIRST';
export const MOST_SPANS = 'MOST_SPANS';
export const LEAST_SPANS = 'LEAST_SPANS';
Loading