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 / Trace Viewer: Implements deep linking to spans #44017

Merged
merged 1 commit into from
Jan 24, 2022

Conversation

erin-noe-payne
Copy link
Contributor

@erin-noe-payne erin-noe-payne commented Jan 13, 2022

@aocenas New PR from my personal fork. Go to town! 😄

What this PR does / why we need it:

Use cases are outlined in #40720.

This PR is doing a lot, so here's a roadmap to explain at a high level what types of changes you are looking at within each grouping of files. I'm also adding some comments in code to assist in navigating.

  • packages/grafana-data/src/types/... Adds the concept of ExplorePanelsState, which allows us to encode the state of explore view panels into the url.
    • This is needed to enable deep linking to a particular datasource & query with the data visualization in a particular state (in this case - with a spanId focused).
    • The data structure maps preferred visualization type to a config object that would be specific to that visualization. This should be extensible for other use cases where you want explore pane visualzations to be configurable & deep linkable, such as @gabor's graph type use case (PR)
  • packages/grafana-data/src/utils/... Serializes ExplorePanelsState into explore url and adds it to internal links.
  • public/app/core/utils/explore.ts Deserializes ExplorePanelsState from explore url
  • packages/jaeger-ui-components/... The UI work to add "focused span" behavior to the trace viewer. Adds the concept of focusedSpanId to the trace viewer components, scrolls the focused span into view and highlights it with a blue background. Knows nothing about URL, explore pane state, etc. Adds the concept of createFocusSpanLink, a function that takes a traceId & spanId and returns a LinkModel representing a deep link to the trace & span.
  • public/app/features/explore/state/... Wire up ExplorePanelsState to redux, keeps it in sync with the url, wires it up to SplitOpen.
  • public/app/features/explore/... Glues together ExplorePanelsState from redux to the Explore UI to the focusedSpanId and createFocusSpanLink features of the trace viewer.

@erin-noe-payne erin-noe-payne requested a review from a team January 13, 2022 21:29
@erin-noe-payne erin-noe-payne requested review from a team as code owners January 13, 2022 21:29
@erin-noe-payne erin-noe-payne requested review from meanmina, mckn, joshhunt and ashharrison90 and removed request for a team January 13, 2022 21:29
@erin-noe-payne erin-noe-payne changed the title Implements deep linking to spans Tempo / Trace Viewer: Implements deep linking to spans Jan 13, 2022
@grafanabot grafanabot added area/explore pr/external This PR is from external contributor area/frontend labels Jan 13, 2022
@aocenas aocenas requested review from aocenas and connorlindsey and removed request for meanmina, joshhunt, ashharrison90 and mckn January 14, 2022 15:32
@aocenas aocenas added this to the 8.4.0 milestone Jan 14, 2022
@connorlindsey connorlindsey added the no-backport Skip backport of PR label Jan 19, 2022
Copy link
Contributor

@connorlindsey connorlindsey left a comment

Choose a reason for hiding this comment

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

LGTM

@aocenas
Copy link
Member

aocenas commented Jan 20, 2022

Looks good but can we add tests to see if the panel state is actually handled correctly? Seems like we always just pass {}. Also, there are some new functions that probably can be easily tested separately. @connorlindsey can you take that on?

@connorlindsey connorlindsey merged commit ac945fb into grafana:main Jan 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants