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

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

Open
ViliusS opened this issue Mar 8, 2024 · 7 comments
Open

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

ViliusS opened this issue Mar 8, 2024 · 7 comments

Comments

@ViliusS
Copy link

ViliusS commented Mar 8, 2024

What happened?

Not sure if this a bug in the UI or in the Jaeger itself, but we have upgraded our Jaeger instances controlled with Jaeger Operator from 1.51 to 1.54. This updated UI from 1.35 to 1.38. Now on at least all-in-one configuration Archive button is enabled by default.

Steps to reproduce

  1. Install Jaeger Operator 1.54
  2. Use this simple YAML to deploy the instance:
apiVersion: jaegertracing.io/v1
kind: Jaeger
metadata:
  name: jaeger
  namespace: default
spec:
  strategy: allInOne
  allInOne:
    options:
      query:
        base-path: /
  ingress:
    ingressClassName: nginx
    hosts:
      - yourhost.example.com
    pathType: Prefix
  storage:
    options:
      memory:
        max-traces: 100000

Expected behavior

Archive button should be disabled by default, as per documentation.

Relevant log output

No response

Screenshot

No response

Additional context

It is probably due changes in #1944.

Jaeger backend version

1.54

SDK

No response

Pipeline

No response

Stogage backend

memory

Operating system

No response

Deployment model

Jaeger Operator for Kubernetes

Deployment configs

apiVersion: jaegertracing.io/v1
kind: Jaeger
metadata:
  name: jaeger
  namespace: default
spec:
  strategy: allInOne
  allInOne:
    options:
      query:
        base-path: /
  ingress:
    ingressClassName: nginx
    hosts:
      - yourhost.example.com
    pathType: Prefix
  storage:
    options:
      memory:
        max-traces: 100000
@ViliusS ViliusS added the bug label Mar 8, 2024
@yurishkuro
Copy link
Member

why is it a problem? The button now reacts to the capability of the underlying backend, if archive is available (as it is with Memory store) then the button shows. I believe there is a UI flag to turn if off.

@ViliusS
Copy link
Author

ViliusS commented Mar 12, 2024

I don't think that that turning on archive button for in-memory backend is correct. The idea that trace can be archived implies that it will be stored permanently. For in-memory backend archived traces won't be persistent between Jaeger restarts though.

@CoryBarney
Copy link

Just as I am going through too see where I can help @ViliusS , maybe a toast message stating the that archive is on volatile storage might be a middle ground? I need to look at how the storage plugin works but it might be enough to just warn as it may very well archive it through the life of the storage but the storage itself being non-persisted is kind of a design issue and not really a functionality issue.

Let me know if I am totally wrong on this as I am just trying to rekindle the discussion as this issue is still open

@ViliusS
Copy link
Author

ViliusS commented Aug 30, 2024

I agree, toast message is better experience than the current state. I don't think it changes much for the end user, though.

If I saw such toast message as an end user (not as sys admin), I would then ask myself: so can I use this archiving feature or should I better just export needed traces for myself locally? If I can use it, how long my archived traces will be stored? Can I rely on on archived traces in let's say compliance investigation cases? None of these questions can be easily answered in case in-memory storage is used so, in the end, as Jeager operator, I would not consider enabling Archive button for in-memory store at all.

Also, independently of what will be decided on this matter the documentation needs to be adjusted here https://www.jaegertracing.io/docs/1.60/frontend-ui/#archive-support . Currently it says that Archive support is disabled by default, but in really it is always enabled.

@yurishkuro
Copy link
Member

The option was changed to default to true in #1944. However, false would probably be the wrong default too, because it can be interpreted as an override on server-returned capability. Ideally, the option should be renamed to archiveDisabled with false meaning "respect backend capability" and true meaning "override backend capability".

@ViliusS
Copy link
Author

ViliusS commented Aug 30, 2024

"backend capability" assumes that there could be a backend which doesn't support archiving, so that logic could still be confusing.

I would suggest leaving the name as it is and just do three options: auto, true and false. This is also more inline with other software.

@yurishkuro
Copy link
Member

"backend capability" assumes that there could be a backend which doesn't support archiving

it's not so much about backend not being able to support archiving (there's nothing to support, archiving is just a regular backend, just a different namespace separate from the main storage), but about the operator configuring jaeger-query with both primary and archive storage.

I wouldn't do auto/true/false - the true option in that list doesn't make sense, because UI does not dictate backend capabilities and so saying true in the UI config but not configuring the backend is a meaningless combination. The only meaningful configuration is UI disabling the Archive button, hence my suggestion to have a property disableArchive.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants