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

Handle Error stored in redux trace.traces #195

Merged
merged 2 commits into from Mar 13, 2018
Merged

Conversation

tiffon
Copy link
Member

@tiffon tiffon commented Mar 12, 2018

Fix #166.

HTTP responses >= 400 (e.g. 404s) cause an Error to be added to the redux store in trace.traces[traceID]. This PR prevents the search page from treating them as a normal trace.

Signed-off-by: Joe Farro <joef@uber.com>
@ghost ghost assigned tiffon Mar 12, 2018
@ghost ghost added the review label Mar 12, 2018
@tiffon
Copy link
Member Author

tiffon commented Mar 12, 2018

@simonrobb can you look this over when you get the chance?

@codecov
Copy link

codecov bot commented Mar 12, 2018

Codecov Report

Merging #195 into master will decrease coverage by 0.03%.
The diff coverage is 75%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #195      +/-   ##
==========================================
- Coverage    91.5%   91.46%   -0.04%     
==========================================
  Files          95       95              
  Lines        2130     2133       +3     
  Branches      434      435       +1     
==========================================
+ Hits         1949     1951       +2     
- Misses        160      161       +1     
  Partials       21       21
Impacted Files Coverage Δ
src/model/search.js 97.29% <75%> (-2.71%) ⬇️

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 2e6d1a4...bafe9a6. Read the comment docs.

@@ -83,7 +86,7 @@ export function getTraceSummary(trace: Trace): TraceSummary {
* @return {TraceSummaries} The `{ traces, maxDuration }` value.
*/
export function getTraceSummaries(_traces: Trace[]): TraceSummaries {
const traces = _traces.map(getTraceSummary);
const traces = _traces.map(getTraceSummary).filter(Boolean);
Copy link
Member

Choose a reason for hiding this comment

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

not sure I am following the typing here: _traces is an array of Trace objects, how could getTraceSummary function be called with an Error object?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's an inefficiency or irregularity in the state of static typing in the repo. Some files have flow-types and some do not.

The type here is really (Trace | Error)[].

Copy link
Member

Choose a reason for hiding this comment

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

so maybe the fix should be (using Scala-ish notation)

_traces.filterNot(_ instanceof Error).map(getTraceSummary);

?

or even do the filtering somewhere upstream, instead of propagating the mixed type deeper.

Choose a reason for hiding this comment

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

Agreed. At the least, if it's possible for there to be Error objects present, the parameter type should be annotated like you described, getTraceSummaries(_traces: (Trace | Error)[]): TraceSummaries.

Copy link
Member Author

Choose a reason for hiding this comment

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

@yurishkuro Great point. I will filter then get the summary. I don't want to filter it further upstream because the presence of the error will be useful in resolving #51.

@simonrobb 👍

Note, the following code was not possible due to Flow flagging it as an error:

const traces = _traces.filter(item => !(item instanceof Error)).map(getTraceSummary);

@@ -83,7 +86,7 @@ export function getTraceSummary(trace: Trace): TraceSummary {
* @return {TraceSummaries} The `{ traces, maxDuration }` value.
*/
export function getTraceSummaries(_traces: Trace[]): TraceSummaries {
const traces = _traces.map(getTraceSummary);
const traces = _traces.map(getTraceSummary).filter(Boolean);

Choose a reason for hiding this comment

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

Agreed. At the least, if it's possible for there to be Error objects present, the parameter type should be annotated like you described, getTraceSummaries(_traces: (Trace | Error)[]): TraceSummaries.

Filter in getTraceSummaries instead of further upstream because the presence of the Error will be useful in resolving #51.

Signed-off-by: Joe Farro <joef@uber.com>
@tiffon tiffon merged commit 6181a29 into master Mar 13, 2018
@ghost ghost removed the review label Mar 13, 2018
@yurishkuro yurishkuro deleted the issue-166-search-page-error branch January 29, 2020 15:07
vvvprabhakar pushed a commit to vvvprabhakar/jaeger-ui that referenced this pull request Jul 5, 2021
* Fix 166 - Handle Error objs in redux trace.traces

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

* Filter for Error objs one level higher

Filter in getTraceSummaries instead of further upstream because the presence of the Error will be useful in resolving #51.

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

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.

None yet

3 participants