-
Notifications
You must be signed in to change notification settings - Fork 521
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
Support trace-scoped external links similar to tag links #480
Conversation
Codecov Report
@@ Coverage Diff @@
## master #480 +/- ##
==========================================
+ Coverage 92.74% 92.79% +0.04%
==========================================
Files 193 194 +1
Lines 4672 4702 +30
Branches 1126 1132 +6
==========================================
+ Hits 4333 4363 +30
Misses 299 299
Partials 40 40
Continue to review full report at Codecov.
|
68b2af2
to
0f27f9a
Compare
If it resolves that issue, please include text Please include sample screenshots. How are these links configured by the end user? Please create a documentation PR to add it to https://www.jaegertracing.io/docs/1.14/frontend-ui/ |
0f27f9a
to
7880b57
Compare
Attached screenshots. |
Can you update the type in |
Added. |
d40409e
to
b4fe753
Compare
@everett980 is this ready to merge? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the pr @rubenvp8510, sorry it took me so long to get to it!
It mostly looks good, aside from simplifying the caching strategy which is admittedly based off of an existing function. That and some minor TS changes and some props that seem unused.
@@ -0,0 +1,47 @@ | |||
// Copyright (c) 2017 Uber Technologies, Inc. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please update year to 2019
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😳
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if this is a brand new file/code, the copyright should be The Jaeger Authors
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@yurishkuro when did this change? should any existing files be updated from "Uber Technologies, Inc." to "The Jaeger Authors"?
Also, the CONTRIBUTING.md#license should probably mention to use the current year, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's always been the guideline. The link also says Jaeger Authors.
Yes, we might fix the year there so that people can copy & paste, although a preferred way is to auto-generate the headers like we do in the other repos (e.g. make fmt
adds them).
|
||
type ExternalLinksProps = { | ||
links: Link[]; | ||
children?: React.ReactNode; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what children
should this component expect? the use case above does not seem to provide children
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rubenvp8510 are there any expected children
? If not, this prop and its consumption in render
can be removed.
e7dffb4
to
fc8dbb9
Compare
I've already address all comments here. Ready for another review! |
2cdb9c0
to
315c94c
Compare
Signed-off-by: Ruben Vargas <[email protected]>
Signed-off-by: Ruben Vargas <[email protected]>
Signed-off-by: Ruben Vargas <[email protected]>
Signed-off-by: Ruben Vargas <[email protected]>
Signed-off-by: Ruben Vargas <[email protected]>
Signed-off-by: Ruben Vargas <[email protected]>
Signed-off-by: Ruben Vargas <[email protected]>
Signed-off-by: Ruben Vargas <[email protected]>
Signed-off-by: Ruben Vargas <[email protected]>
07df312
to
4c2427f
Compare
Done! |
Signed-off-by: Ruben Vargas <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! 👍
🎉 |
Is there an example on how to configure this? |
Any example or documentation around this would help. We are planning to use this. |
Sorry, I haven't had a chance to write documentation about this. I'll send a PR today. You can add trace links to your traces adding an entry into here is an example:
Both url and text can be defined as templates (i.e. using #{field-name}) (same as other linkPatterns). Supported template fields: |
Support trace-scoped external links similar to tag links Signed-off-by: vvvprabhakar <[email protected]>
Signed-off-by: Ruben Vargas [email protected]
Which problem is this PR solving?
Short description of the changes