Make ArchiveTrace button auto-configurable #1944

thecoons · 2023-11-05T14:23:30Z

Resolves [Feature] Make ArchiveTrace button in the UI auto-configurable jaeger#4874

The button to archive a trace is now configured based on the state of
the QueryService in addition to the UI configuration.

All corresponding tests have been updated.

I have read
https://github.com/jaegertracing/jaeger/blob/master/CONTRIBUTING_GUIDELINES.md
I have signed all commits
I have added unit tests for the new functionality
I have run lint and test steps successfully

Signed-off-by: Barthelemy Antonin [email protected]

codecov · 2023-11-05T17:18:40Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Files	Coverage Δ
...kages/jaeger-ui/src/components/TracePage/index.tsx	`99.43% <100.00%> (+<0.01%)`	⬆️
...ackages/jaeger-ui/src/constants/default-config.tsx	`75.00% <ø> (ø)`
packages/jaeger-ui/src/utils/config/get-config.tsx	`100.00% <100.00%> (ø)`

📢 Thoughts on this report? Let us know!

packages/jaeger-ui/src/utils/config/get-config.tsx

## Which problem is this PR solving? - backend part of #4874 ## Description of the changes The button to archive a trace is now configured based on the state of the QueryService in addition to the UI configuration. It is now possible to request features from the QueryService to inject them into the UI. Related UI change jaegertracing/jaeger-ui#1944 ## How was this change tested? All corresponding tests have been updated. ## Checklist - [X] I have read https://github.com/jaegertracing/jaeger/blob/master/CONTRIBUTING_GUIDELINES.md - [X] I have signed all commits - [X] I have added unit tests for the new functionality - [X] I have run lint and test steps successfully --------- Signed-off-by: Antonin Barthelemy <[email protected]> --------- Signed-off-by: Barthelemy Antonin <[email protected]> Signed-off-by: Yuri Shkuro <[email protected]> Co-authored-by: Yuri Shkuro <[email protected]> Co-authored-by: Yuri Shkuro <[email protected]>

thecoons · 2023-11-09T13:21:26Z

When I use the command make run-all-in-one, the queryservice is configured with an in-memory storage, yet when retrieving the capabilities, it shows archiveStorage as false. Does this storage indeed have a non-null reader/writer?

yurishkuro · 2023-11-09T18:20:14Z

I see the issue. This commit yurishkuro/jaeger@9f0e622 modifies the placeholder for the UI to look similar to the index.html in this UI PR. With that change if you run go run ./cmd/all-in-one you should get a placeholder HTML page but with variables substituted accordingly. However, this is the substitution that actually happens:

<title>Test Page</title>
  | <!--
  | This comment emulates the corresponding content in jaeger-ui/packages/jaeger-ui/index.html
  | which contains placeholders that are replaced on load by the query service in order to customize
  | the UI configuration.
  |  
  | // JAEGER_CONFIG_JS
  |  
  | JAEGER_CONFIG = DEFAULT_CONFIG;
  |  
  | JAEGER_COMPATIBILITIES = DEFAULT_COMPATIBILITIES;
  |  
  | JAEGER_VERSION = {"gitCommit":"","gitVersion":"","buildDate":""};
  | -->

Note that verison was replaced, but capabilities were not. It's because your regex pattern in the Go code is using different names than in the UI index.html

Suggestion:

apply the change from the above commit to Go code Make UI placeholder more descriptive jaeger#4937
change the unit test to use the placeholder html instead of cmd/query/app/fixture/index.html so that we can ensure that we're using the same strings
up to you if you want to keep the vars as JAEGER_COMPATIBILITIES or JAEGER_STORAGE_COMPATIBILITIES (I prefer the latter), but they must be consistent between the backend and the UI.
- It would be nice if we could actually guarantee somehow that the strings are always the same, but I don't have a suggestion how to achieve that robustly. At minimum we need to ensure that the strings are identical between ui/index.html and cmd/query/app/ui/placeholder/index.html (and remove cmd/query/app/fixture/index.html altogether)

thecoons · 2023-11-10T11:45:05Z

It would be nice if we could actually guarantee somehow that the strings are always the same, but I don't have a suggestion how to achieve that robustly.

I think the best approach would be to separate the query service from the UI and have them communicate through an API. Currently, I don't see a way to ensure consistency between the backend and the UI without creating a house of cards.

thecoons · 2023-11-10T12:15:29Z

I trapped myself with the variable naming....
I made the changes and the feature is functional. The pull request is open.

change the unit test to use the placeholder html instead of cmd/query/app/fixture/index.html so that we can ensure that we're using the same strings

I will open a pull request for this change.

packages/jaeger-ui/src/utils/config/get-config.tsx

yurishkuro · 2023-11-11T19:10:54Z

packages/jaeger-ui/src/utils/config/get-config.test.js

+    let capabilities;
+    let getJaegerStorageCapabilities;


why do these need to be made global within the tests instead of being local vars inside beforeEach function? Only embedded is used in some tests.

Alternatively, we should use capabilities in all the expected fields because when I read the tests, it is technically possible for it to be overridden by a field in embedded.

ex: 'gives precedence to non-objects in embedded'

thecoons · 2023-11-11T19:13:18Z

I have implemented the suggestions you had made.
Please don't hesitate to point out anything that seems to need further work.

packages/jaeger-ui/src/utils/config/get-config.test.js

- Resolves #4874 The button to archive a trace is now configured based on the state of the QueryService in addition to the UI configuration. All corresponding tests have been updated. - [X] I have read https://github.com/jaegertracing/jaeger/blob/master/CONTRIBUTING_GUIDELINES.md - [X] I have signed all commits - [X] I have added unit tests for the new functionality - [X] I have run lint and test steps successfully --------- Signed-off-by: Barthelemy Antonin <[email protected]>

yurishkuro

thanks!

Signed-off-by: Yuri Shkuro <[email protected]>

thecoons requested a review from a team as a code owner November 5, 2023 14:23

thecoons requested review from jkowall and removed request for a team November 5, 2023 14:23

thecoons marked this pull request as draft November 5, 2023 14:23

thecoons force-pushed the make_archivetrace_auto_config branch from 8d947e7 to 501157e Compare November 5, 2023 14:36

yurishkuro reviewed Nov 5, 2023

View reviewed changes

packages/jaeger-ui/src/utils/config/get-config.tsx Outdated Show resolved Hide resolved

yurishkuro added the changelog:new-feature Change that should be called out as new feature in CHANGELOG label Nov 5, 2023

yurishkuro mentioned this pull request Nov 7, 2023

Make ArchiveTrace button auto-configurable jaegertracing/jaeger#4913

Merged

4 tasks

yurishkuro reviewed Nov 7, 2023

View reviewed changes

packages/jaeger-ui/src/utils/config/get-config.tsx Show resolved Hide resolved

yurishkuro reviewed Nov 7, 2023

View reviewed changes

packages/jaeger-ui/src/utils/config/get-config.tsx Outdated Show resolved Hide resolved

thecoons force-pushed the make_archivetrace_auto_config branch 2 times, most recently from 14cb59b to 7f269a6 Compare November 9, 2023 13:12

thecoons force-pushed the make_archivetrace_auto_config branch from 7f269a6 to 184438f Compare November 10, 2023 11:36

thecoons marked this pull request as ready for review November 10, 2023 11:38

thecoons force-pushed the make_archivetrace_auto_config branch from 184438f to 75e41d3 Compare November 10, 2023 12:05

thecoons force-pushed the make_archivetrace_auto_config branch from 75e41d3 to a93c014 Compare November 10, 2023 14:13

yurishkuro reviewed Nov 10, 2023

View reviewed changes

packages/jaeger-ui/src/utils/config/get-config.tsx Outdated Show resolved Hide resolved

yurishkuro reviewed Nov 10, 2023

View reviewed changes

packages/jaeger-ui/src/utils/config/get-config.tsx Outdated Show resolved Hide resolved

yurishkuro reviewed Nov 10, 2023

View reviewed changes

packages/jaeger-ui/src/utils/config/get-config.tsx Outdated Show resolved Hide resolved

thecoons force-pushed the make_archivetrace_auto_config branch 3 times, most recently from 056a576 to 2bef1d9 Compare November 11, 2023 19:06

yurishkuro reviewed Nov 11, 2023

View reviewed changes

packages/jaeger-ui/src/utils/config/get-config.test.js Outdated Show resolved Hide resolved

thecoons force-pushed the make_archivetrace_auto_config branch from 2bef1d9 to a924197 Compare November 11, 2023 19:28

yurishkuro reviewed Nov 11, 2023

View reviewed changes

packages/jaeger-ui/src/utils/config/get-config.test.js Outdated Show resolved Hide resolved

thecoons force-pushed the make_archivetrace_auto_config branch from a924197 to 86c8cb3 Compare November 11, 2023 19:31

thecoons force-pushed the make_archivetrace_auto_config branch from 86c8cb3 to 676c671 Compare November 11, 2023 19:36

yurishkuro previously approved these changes Nov 11, 2023

View reviewed changes

yurishkuro enabled auto-merge (squash) November 11, 2023 23:12

yarn prettier

caf070d

Signed-off-by: Yuri Shkuro <[email protected]>

yurishkuro dismissed their stale review via caf070d November 11, 2023 23:16

yurishkuro previously approved these changes Nov 11, 2023

View reviewed changes

delint

ead543d

Signed-off-by: Yuri Shkuro <[email protected]>

yurishkuro dismissed their stale review via ead543d November 11, 2023 23:36

yurishkuro approved these changes Nov 11, 2023

View reviewed changes

yurishkuro merged commit 8a88b39 into jaegertracing:main Nov 11, 2023
10 checks passed

ViliusS mentioned this pull request Mar 8, 2024

[Bug]: Archive button is enabled by default #2201

Open

thecoons deleted the make_archivetrace_auto_config branch December 22, 2024 11:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make ArchiveTrace button auto-configurable #1944

Make ArchiveTrace button auto-configurable #1944

thecoons commented Nov 5, 2023 •

edited

Loading

codecov bot commented Nov 5, 2023 •

edited

Loading

thecoons commented Nov 9, 2023 •

edited

Loading

yurishkuro commented Nov 9, 2023 •

edited

Loading

thecoons commented Nov 10, 2023

thecoons commented Nov 10, 2023 •

edited

Loading

yurishkuro Nov 11, 2023

thecoons Nov 11, 2023 •

edited

Loading

thecoons commented Nov 11, 2023

yurishkuro left a comment

Make ArchiveTrace button auto-configurable #1944

Make ArchiveTrace button auto-configurable #1944

Conversation

thecoons commented Nov 5, 2023 • edited Loading

codecov bot commented Nov 5, 2023 • edited Loading

Codecov Report

thecoons commented Nov 9, 2023 • edited Loading

yurishkuro commented Nov 9, 2023 • edited Loading

thecoons commented Nov 10, 2023

thecoons commented Nov 10, 2023 • edited Loading

yurishkuro Nov 11, 2023

Choose a reason for hiding this comment

thecoons Nov 11, 2023 • edited Loading

Choose a reason for hiding this comment

thecoons commented Nov 11, 2023

yurishkuro left a comment

Choose a reason for hiding this comment

thecoons commented Nov 5, 2023 •

edited

Loading

codecov bot commented Nov 5, 2023 •

edited

Loading

thecoons commented Nov 9, 2023 •

edited

Loading

yurishkuro commented Nov 9, 2023 •

edited

Loading

thecoons commented Nov 10, 2023 •

edited

Loading

thecoons Nov 11, 2023 •

edited

Loading