Skip to content

Conversation

JamesNK
Copy link
Member

@JamesNK JamesNK commented Oct 12, 2024

Description

The traces page shows a summary of info about it's spans apps . Spans are grouped by app, and then the apps are ordered by its earliest span.

The problem with this is span start dates can be out of order. For example, the start time reported by a .NET app and a browser app could be slightly off, causing a child browser app span to be earlier than its parent server span.

This PR changes the ordering logic so a child can't be ordered before its parent. There are performance improvements that could be made to here, but the old logic wasn't optimized either. Only runs for traces than are rendered to the screen which is limited by virtualization.

Low priority. Post 9.0

Checklist

  • Is this feature complete?
    • Yes. Ready to ship.
    • No. Follow-up changes expected.
  • Are you including unit tests for the changes and scenario tests if relevant?
    • Yes
    • No
  • Did you add public API?
    • Yes
      • If yes, did you have an API Review for it?
        • Yes
        • No
      • Did you add <remarks /> and <code /> elements on your triple slash comments?
        • Yes
        • No
    • No
  • Does the change make any security assumptions or guarantees?
    • Yes
      • If yes, have you done a threat model and had a security review?
        • Yes
        • No
    • No
  • Does the change require an update in our Aspire docs?
    • Yes
      • Link to aspire-docs issue:
    • No
Microsoft Reviewers: Open in CodeFlow

@JamesNK JamesNK added area-dashboard NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) labels Oct 12, 2024
@JamesNK JamesNK requested review from drewnoakes and adamint October 12, 2024 01:16
@JamesNK JamesNK force-pushed the jamesnk/order-traces branch from 26593b8 to 3b5ee13 Compare October 21, 2024 01:24
@JamesNK JamesNK force-pushed the jamesnk/order-traces branch from 3b5ee13 to 8b848b0 Compare October 21, 2024 13:20
@JamesNK JamesNK removed the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Oct 21, 2024
@JamesNK JamesNK requested a review from adamint October 21, 2024 13:31
@JamesNK
Copy link
Member Author

JamesNK commented Oct 21, 2024

Many changes. Please take another look.

@JamesNK
Copy link
Member Author

JamesNK commented Oct 23, 2024

@drewnoakes @adamint

@JamesNK JamesNK enabled auto-merge (squash) November 12, 2024 01:40
@JamesNK JamesNK merged commit 99341c9 into main Nov 12, 2024
9 checks passed
@JamesNK JamesNK deleted the jamesnk/order-traces branch November 12, 2024 03:05
@JamesNK JamesNK mentioned this pull request Nov 18, 2024
18 tasks
@github-actions github-actions bot locked and limited conversation to collaborators Dec 12, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants