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

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

Closed
yurishkuro opened this issue Oct 21, 2023 · 7 comments · Fixed by jaegertracing/jaeger-ui#1944
Closed
Labels
good first issue Good for beginners help wanted Features that maintainers are willing to accept but do not have cycles to implement

Comments

@yurishkuro
Copy link
Member

When archive storage is configured in the query-service it can receive POST requests from the UI with a trace ID and clone that trace from primary storage into archive, useful for longer retention. However, the Archive Trace button has to be separately enabled in the UI for that, even though the query-service can check at runtime if archive storage is available.

Proposal

When query-service embeds the UI config into UI loading page, it can also include a structure describing the capabilities of the backend. One of the flags in this structure would be whether the archive storage is available, so that users wouldn't need to configure it separately in the UI. The same struct can be used to inform the UI of other things, for example whether more flexible queries are supported by the storage, like regex on tags (supported in ES but not in Cassandra).

We should still keep the enableArchive in the main UI config, but use it as a kill switch / override for archive function. When the UI decides whether to show the button, it would be backendSupportsArchive && config.archiveEnabled.

Implementing this ticket requires some knowledge of both Go and Node.js

Details, Pointers

The query-service injects a config into UI loading page in the static handler:

if configObject, err := loadUIConfig(options.UIConfigPath); err != nil {

@yurishkuro yurishkuro added help wanted Features that maintainers are willing to accept but do not have cycles to implement good first issue Good for beginners labels Oct 21, 2023
@MeenuyD
Copy link
Contributor

MeenuyD commented Oct 21, 2023

Hello @yurishkuro I would like to work on this issue can you please tell should I change the logic to this

capabilities := BackendCapabilities{
        SupportsArchive: true,  
        SupportsFlexibleQueries: true,  
    }
    uiConfig.BackendCapabilities = capabilities
    uiConfigJSON, err := json.Marshal(uiConfig)
    if err != nil {
        return nil, fmt.Errorf("failed to marshal UI config: %w", err)
    }
    indexBytes = configObject.regexp.ReplaceAll(indexBytes, uiConfigJSON)

    return indexBytes, nil
```  by creating some new variables?
And the frontend should be like this ?
const { supportsArchive, supportsFlexibleQueries } = data.backendCapabilities;
        if (supportsArchive) {
            enableArchiveButton();
        } else {
            disableArchiveButton();
        }

@yurishkuro
Copy link
Member Author

on a high level - yes, but many details are wrong. You need to try in live code.

@MeenuyD
Copy link
Contributor

MeenuyD commented Oct 21, 2023

Ok you assign me I will try and correct it

@yurishkuro
Copy link
Member Author

we don't assign tickets, but feel free to implement

thecoons added a commit to thecoons/jaeger that referenced this issue Oct 31, 2023
## Which problem is this PR solving?
- Resolves jaegertracing#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.

## 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]>
thecoons added a commit to thecoons/jaeger that referenced this issue Oct 31, 2023
- Resolves jaegertracing#4874

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.

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]>
thecoons added a commit to thecoons/jaeger that referenced this issue Nov 1, 2023
- Resolves jaegertracing#4874

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.

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]>
thecoons added a commit to thecoons/jaeger that referenced this issue Nov 3, 2023
- Resolves jaegertracing#4874

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.

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]>
thecoons added a commit to thecoons/jaeger that referenced this issue Nov 3, 2023
- Resolves jaegertracing#4874

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.

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]>
thecoons added a commit to thecoons/jaeger that referenced this issue Nov 4, 2023
- Resolves jaegertracing#4874

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.

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]>
thecoons added a commit to thecoons/jaeger that referenced this issue Nov 5, 2023
- Resolves jaegertracing#4874

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.

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]>
@thecoons
Copy link
Contributor

thecoons commented Nov 5, 2023

I have made the requested changes on the jaeger PR and pushed one on jaeger-ui. Currently, I'm still having some issues testing the integration, but this allows for a preview of the end-to-end implementation. I m still working on both.

yurishkuro added a commit that referenced this issue Nov 7, 2023
## 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]>
yurishkuro added a commit that referenced this issue Nov 9, 2023
## Which problem is this PR solving?
- Part of #4874

## Description of the changes
- Add some styling to the placeholder
- Make the config substitutions directly visible
- Add missing capabilities var

## How was this change tested?
`go run ./cmd/all-in-one`

<img width="902" alt="image"
src="https://github.com/jaegertracing/jaeger/assets/3523016/0e08229c-f1f0-45cb-8e3f-e7f4daaa5337">

---------

Signed-off-by: Yuri Shkuro <[email protected]>
yurishkuro pushed a commit to jaegertracing/jaeger-ui that referenced this issue Nov 11, 2023
- Resolves jaegertracing/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.

- [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]>

Signed-off-by: Barthelemy Antonin <[email protected]>
@yurishkuro
Copy link
Member Author

@thecoons Thank you for your contribution! 🎉 🎉 🎉 And for sticking with it during my picky code reviews :-)

@thecoons
Copy link
Contributor

@yurishkuro I expected no less from you, and it's a pleasure to contribute to Jaeger.

P.S.: Your book is a very interesting read :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for beginners help wanted Features that maintainers are willing to accept but do not have cycles to implement
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants