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

Collapsable trace tree #312

Merged
merged 4 commits into from
Jan 9, 2025
Merged

Collapsable trace tree #312

merged 4 commits into from
Jan 9, 2025

Conversation

skull8888888
Copy link
Collaborator

@skull8888888 skull8888888 commented Jan 9, 2025

Important

Add collapsible trace tree feature in frontend and MachineManager feature flag in backend.

  • Frontend:
    • Add collapsible trace tree functionality in span-card.tsx, timeline.tsx, and trace-view.tsx.
    • Introduce collapsedSpans state to manage collapsed spans.
    • Update SpanCard and Timeline components to handle span collapse/expand actions.
  • Backend:
    • Add MachineManager feature flag in features/mod.rs.
    • Update main.rs to use MachineManager feature flag instead of FullBuild for machine manager initialization.
  • Misc:
    • Update posthog-js version in package.json.

This description was created by Ellipsis for 0dfb838. It will automatically update as commits are pushed.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

👍 Looks good to me! Reviewed everything up to 3009424 in 36 seconds

More details
  • Looked at 254 lines of code in 6 files
  • Skipped 1 files when reviewing.
  • Skipped posting 5 drafted comments based on config settings.
1. app-server/src/features/mod.rs:32
  • Draft comment:
    Ensure that the environment variable MACHINE_MANAGER_URL_GRPC is documented and set appropriately in all environments where this feature might be used.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The PR introduces a new feature flag MachineManager and updates the logic to check for this feature using the environment variable MACHINE_MANAGER_URL_GRPC. This is consistent with the existing pattern for feature flags in the codebase.
2. app-server/src/main.rs:296
  • Draft comment:
    Ensure that the MACHINE_MANAGER_URL_GRPC environment variable is set in environments where the MachineManager feature is enabled.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The PR updates the feature flag check in main.rs to use the new MachineManager feature. This is consistent with the changes in features/mod.rs.
3. frontend/components/traces/span-card.tsx:22
  • Draft comment:
    Consider adding PropTypes or TypeScript interfaces to ensure collapsedSpans and onToggleCollapse are passed correctly to SpanCard.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The PR introduces a new feature to collapse spans in the trace view. The SpanCard component now accepts collapsedSpans and onToggleCollapse props to manage the collapsed state of spans.
4. frontend/components/traces/timeline.tsx:44
  • Draft comment:
    Ensure that collapsedSpans is correctly managed and updated to reflect the current state of the UI.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The Timeline component now considers collapsedSpans to determine which spans to display. This is consistent with the new feature to collapse spans.
5. frontend/components/traces/trace-view.tsx:53
  • Draft comment:
    Consider adding a function to reset collapsedSpans when the trace data changes to ensure the UI reflects the current data accurately.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The TraceView component manages the state of collapsedSpans and passes it to SpanCard and Timeline. This is consistent with the new feature to collapse spans.

Workflow ID: wflow_tzDDPTaFTRpqnT78


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

❌ Changes requested. Incremental review on 75a2ac5 in 39 seconds

More details
  • Looked at 20 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 0 drafted comments based on config settings.

Workflow ID: wflow_RG7BnqwnqEO1Py8R


Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

setTimelineWidth(
container.current!.getBoundingClientRect().width
);
setTimeout(() => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Using setTimeout with a delay of 10ms is not a reliable way to ensure state updates before URL changes. Consider using a callback or a state effect to handle this synchronously.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

👍 Looks good to me! Incremental review on 0dfb838 in 45 seconds

More details
  • Looked at 53 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 drafted comments based on config settings.
1. frontend/components/traces/timeline.tsx:51
  • Draft comment:
    The dependency array for useCallback is incomplete. It should include childSpans as it is used within the traverse function.
    [collapsedSpans, childSpans]
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable:
    The key insight is that childSpans is passed as an argument to traverse, not accessed from the outer scope. React's useCallback is only needed for dependencies accessed from closure scope. Arguments passed directly to the function don't need to be in the dependency array. Only collapsedSpans is accessed from closure scope.
    Could there be a performance benefit to memoizing based on childSpans changes? Could there be edge cases where childSpans closure capture matters?
    No - since childSpans is passed as an argument, the current implementation is correct. Adding unnecessary dependencies would actually hurt performance by causing more re-renders.
    The comment should be deleted. The current dependency array is correct since childSpans is passed as an argument and not accessed from closure scope.

Workflow ID: wflow_D9vD9H6I67DxaZBZ


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

@skull8888888 skull8888888 merged commit 8114f11 into dev Jan 9, 2025
3 checks passed
@skull8888888 skull8888888 deleted the collapsable branch January 9, 2025 11:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant