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

Tempo: Don't modify the passed time range when using timeShiftEnabled #87980

Merged
merged 2 commits into from
May 21, 2024

Conversation

aocenas
Copy link
Member

@aocenas aocenas commented May 16, 2024

Fixes: #87608

The moment methods to modify the date return an object so it seems like they should be immutable but actually also modify the date object. So if the object is passed for example from Explore we modify the explore state as well without going through the correct redux updates which creates issues.

Copy link
Contributor

@joey-grafana joey-grafana left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't seem to be updating the time in the opened right panel in the UI.

request.range is updated but maybe we also need to update the raw value in request.range. I've also noticed there's a request.rawRange which I'm not sure why we're doing this!

Although the UI is not being updated so maybe it's a Redux state/update issue.

@aocenas
Copy link
Member Author

aocenas commented May 20, 2024

@joey-grafana

And do we want to do that? Like there are 2 cases

  1. clicking from the traces search to open a single trace through data link. In that case it should have been implemented inside the data link and not where it is right now
  2. we seem to apply this for traceID type of query just for sure I guess, to make sure user gets a a full trace. In this case it should not change the time range, like then every time you run a query the time range gets wider which imho seems pretty odd and does not actually help (because without knowing you are making the query slower without added benefit).

And then if we want and have 2. then I don't think we need 1.

@aocenas
Copy link
Member Author

aocenas commented May 20, 2024

request.range is updated but maybe we also need to update the raw value in request.range. I've also noticed there's a request.rawRange which I'm not sure why we're doing this!

Although the UI is not being updated so maybe it's a Redux state/update issue.

I am not sure I understand this but this is not how redux works. Like updating an object value directly that is part of state whether it's redux or react state is a bug (unless it's a Ref type). You create a weird undefined behaviour as you are skipping any rerendering or onChange hooks and reducers etc.

@aocenas aocenas merged commit a2aea70 into main May 21, 2024
14 checks passed
@aocenas aocenas deleted the aocenas/tempo/fix-range-modification branch May 21, 2024 11:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Tempo trace query is recursively shifting the time on every new trace search
2 participants