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

Add View Option for raw/unadjusted trace #153

Merged
merged 4 commits into from Dec 24, 2017

Conversation

Projects
None yet
3 participants
@yurishkuro
Copy link
Member

commented Dec 23, 2017

Fixes #152

Add View Option for raw/unadjusted trace
Signed-off-by: Yuri Shkuro <ys@uber.com>

@yurishkuro yurishkuro requested a review from tiffon Dec 23, 2017

@@ -104,7 +104,12 @@ export default function TracePageHeader(props: TracePageHeaderProps) {
<Dropdown.Menu>
<Dropdown.Item>
<a rel="noopener noreferrer" target="_blank" href={`/api/traces/${traceID}`}>

This comment has been minimized.

Copy link
@yurishkuro

yurishkuro Dec 23, 2017

Author Member

@tiffon I don't see this file in #151, it probably needs adjustment with the prefix as well

This comment has been minimized.

Copy link
@tiffon

tiffon Dec 23, 2017

Member

@yurishkuro Yes, this needs the prefixUrl(...).

Also, is the trailing / in the raw version URL intentional?

/api/traces/${traceID}/?raw=true

Seems like the only difference should be the query string is appended?

/api/traces/${traceID}?raw=true
@codecov

This comment has been minimized.

Copy link

commented Dec 23, 2017

Codecov Report

Merging #153 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #153   +/-   ##
=======================================
  Coverage   92.66%   92.66%           
=======================================
  Files          85       85           
  Lines        1881     1881           
  Branches      366      366           
=======================================
  Hits         1743     1743           
  Misses        126      126           
  Partials       12       12
Impacted Files Coverage Δ
src/components/TracePage/TracePageHeader.js 92.85% <ø> (ø) ⬆️
src/components/SearchTracePage/TraceSearchForm.js 86.51% <100%> (ø) ⬆️

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 8d673fa...fa904d1. Read the comment docs.

@black-adder

This comment has been minimized.

Copy link
Contributor

commented Dec 23, 2017

add this to the change log so it can go out with 1.1?

@tiffon
Copy link
Member

left a comment

Requesting change of raw URL to not the trailing slash .../${traceID}/ or confirmation that it is intentional.

yurishkuro and others added some commits Dec 24, 2017

Remove /
Signed-off-by: Yuri Shkuro <ys@uber.com>
Add missing prefixUrl adjustment
Signed-off-by: Joe Farro <joef@uber.com>
Fix failing unit test
Signed-off-by: Joe Farro <joef@uber.com>
@tiffon

This comment has been minimized.

Copy link
Member

commented Dec 24, 2017

@yurishkuro The prefixUrl(...) needs to be in master in the event of package.json#homepage is changed; it's not just relevant to the query-config PR (#151).

I went ahead and added it (and fixed a failing unit test).

@tiffon

tiffon approved these changes Dec 24, 2017

@tiffon tiffon merged commit 0e00d93 into master Dec 24, 2017

1 of 4 checks passed

License Compliance FOSSA analyzing commit
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
continuous-integration/travis-ci/push The Travis CI build is in progress
Details
DCO All commits have a DCO sign-off from the author
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.