Allow a tag to be displayed next to operation name (revamp #487) #703
Conversation
Signed-off-by: Dimitrios Kouzis-Loukas <[email protected]>
`opLabel` is either false or a populated string. Signed-off-by: Bryan Boreham <[email protected]>
Suggested by https://github.com/everett980 Signed-off-by: Bryan Boreham <[email protected]>
Suggested by https://github.com/everett980 Signed-off-by: Bryan Boreham <[email protected]>
Signed-off-by: Bryan Boreham <[email protected]>
Suggested by https://github.com/everett980: By replacing `renderPattern` with a function that returns all parameter values (or returns `false`) that function can be reused without creating a partial `ProcessedLinkPattern` in `computeSingleTagPattern`. Signed-off-by: Bryan Boreham <[email protected]>
bboreham
requested review from
everett980,
rubenvp8510 and
tiffon
as code owners
Signed-off-by: Bryan Boreham <[email protected]>
Lint pointed this out Signed-off-by: Bryan Boreham <[email protected]>
Codecov Report
@@ Coverage Diff @@
## master #703 +/- ##
==========================================
+ Coverage 94.39% 94.40% +0.01%
==========================================
Files 230 230
Lines 5959 5970 +11
Branches 1448 1454 +6
==========================================
+ Hits 5625 5636 +11
Misses 300 300
Partials 34 34
Continue to review full report at Codecov.
|
|
@th3M1ke would you be able to review this? |
|
@yurishkuro sure! will review later today |
| onMissingFn?: (_: string) => void | ||
| ) { | ||
| const parameterValues: Record<string, any> = {}; | ||
| const allParameters = parameters.every(parameter => { |
There was a problem hiding this comment.
Why we need allParameters?
| onMissingFn?: (_: string) => void | ||
| ) { | ||
| const parameterValues: Record<string, any> = {}; | ||
| const allParameters = parameters.every(parameter => { |
There was a problem hiding this comment.
| const allParameters = parameters.every(parameter => { | |
| parameters.forEach(parameter => { |
To be able to render different paraments(from different scopes. HTTP calls statuses and Redis's driverID, for example)
There was a problem hiding this comment.
This code is from @everett980 at https://github.com/jaegertracing/jaeger-ui/pull/487/files#r356227730.
What happens to return values if I make that change?
Are you proposing that the "all elements match otherwise nothing" behaviour be changed?
That was quite deliberate in the original PR; @lookfwd added a test for it
|
What's the state of this PR? |
|
I was awaiting further comment. |
|
@rubenvp8510 could you please review this one? |
|
This is quite an old what, any chance of this getting revived? I'd be happy to help out |
|
It's unlikely that I would be able to put a lot of time into it, but I was only tidying up other people's work; very happy if you want to pick it up. |
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. You may re-open it if you need more time. |
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
| const viewBounds = getViewedBounds(span.startTime, span.startTime + span.duration); | ||
| const viewStart = viewBounds.start; | ||
| const viewEnd = viewBounds.end; | ||
| const opLabelPattern = getConfigValue('opLabel'); |
There was a problem hiding this comment.
There appears to be a configuration key mismatch that will prevent this feature from working correctly. The PR description and tests reference opLabelTag as the configuration key, but the implementation is using getConfigValue('opLabel') and the default config defines opLabel: null.
To ensure the feature works as intended, please change:
const opLabelPattern = getConfigValue('opLabel');to:
const opLabelPattern = getConfigValue('opLabelTag');Additionally, the default config should be updated to match this naming convention for consistency.
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
| ); | ||
| } | ||
|
|
||
| export function computeSingleTagPattern(pattern: String, span: Span) { |
There was a problem hiding this comment.
The type annotation for pattern should be string (primitive) rather than String (object wrapper). This mismatch could cause runtime type errors since the function receives string primitives but expects String objects. Changing String to string will ensure proper TypeScript typing.
| export function computeSingleTagPattern(pattern: String, span: Span) { | |
| export function computeSingleTagPattern(pattern: string, span: Span) { |
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
| expect(wrapper.find('.endpoint-name').text()).toBe(props.rpc.operationName); | ||
| }); | ||
|
|
||
| it('has expected lebel when no rpc', () => { |
There was a problem hiding this comment.
There's a small typo in the test description: lebel should be label. While this is just a test description, typos in test names can make it harder to understand test coverage when reviewing or debugging tests.
| it('has expected lebel when no rpc', () => { | |
| it('has expected label when no rpc', () => { |
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. You may re-open it if you need more time. |
|
This pull request has been automatically closed due to inactivity. You may re-open it if you need more time. We really appreciate your contribution and we are sorry that this has not been completed. |
6 participants
This PR takes the change in #487 from @lookfwd, rebases it against latest, and applies the suggested changes from @everett980.
I signed the extra commits as myself because I don't want to impersonate someone without permission, but the code is from @everett980. Some small changes were needed to let tests pass.