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

Change outgoing request to arrow in trace span #1684

Merged

Conversation

adamint
Copy link
Member

@adamint adamint commented Jan 17, 2024

Changes the trace bar into ArrowCircleRight icon for outgoing requests, as well as changes the uninstrumented peer icon from ArrowCircleRight to ArrowBounce (the fact that it's an outgoing request is also captured by its parent's icon)

image

resolves #1675

Microsoft Reviewers: Open in CodeFlow

@adamint adamint changed the title Change outgoing request to arrow in trace span, change uninstrumented… Change outgoing request to arrow in trace span Jan 17, 2024
@DamianEdwards
Copy link
Member

I like the idea of using a different arrow to represent uninstrumented hops. Not 100% sure of the bounce arrow though. Are there other arrow options available?

@adamint
Copy link
Member Author

adamint commented Jan 17, 2024

@DamianEdwards you can find all options here by searching arrow

https://fluenticons.co/

Maybe?
image

@DamianEdwards
Copy link
Member

@adamint yeah that's exactly what I was thinking too. I like that one for this use case.

@ilmax
Copy link

ilmax commented Jan 17, 2024

Maybe not very serious but I was thinking about something similar to a shrug emoji to signal that the call is not instrumented 😅

@adamint
Copy link
Member Author

adamint commented Jan 17, 2024

@adamint yeah that's exactly what I was thinking too. I like that one for this use case.

Changed to ArrowExportLtr
image

@JamesNK
Copy link
Member

JamesNK commented Jan 17, 2024

Indentation needs to be consistent. See the different here:

image

@JamesNK
Copy link
Member

JamesNK commented Jan 17, 2024

I prefer the old icon in two places. I used it in the mockup to show that a request is being made to the server, regardless of whether it is a request to an instrumented server or a request to an uninstrumented server.

Another problem with new icons in this PR is they don't show the color well.

Why do we want different icons? If we do want different icons then I'd prefer the old outbound arrow icon to remain unchanged, and the new icon to the left to be some kind of server icon.

@mitchdenny mitchdenny added this to the preview 3 (Feb) milestone Jan 18, 2024
@adamint
Copy link
Member Author

adamint commented Jan 18, 2024

I'm confused at

they don't show the color well

This is the same icon as in your mockup.

I do think it's worth keeping the instrumented / uninstrumented call icon separate, as they are different kinds of requests. I prefer keeping the icons the same as in PR, though I think we are both expressing a subjecrtive preference.

I can ensure the padding is consistent, but otherwise I think this is good.

@JamesNK
Copy link
Member

JamesNK commented Jan 18, 2024

I'm confused at

they don't show the color well

This is the same icon as in your mockup.

This one:

image

I used an arrow will a solid background color on purpose, so it was easy to see the color.

I do think it's worth keeping the instrumented / uninstrumented call icon separate, as they are different kinds of requests. I prefer keeping the icons the same as in PR, though I think we are both expressing a subjecrtive preference.

I can ensure the padding is consistent, but otherwise I think this is good.

I think I'm ok with them being different, but I'd like to find good icons.

@JamesNK
Copy link
Member

JamesNK commented Jan 18, 2024

How about this:

For servers, we use the server icon:

image

And continue to use ArrowCircleRight for the uninstrumented call icon.


I like this solution because I think in the future, we will use different icons based on the server type. e.g. web server would use server, DB server icon for SQL server/Postgres/etc.

image

@DamianEdwards
Copy link
Member

I like this solution because I think in the future, we will use different icons based on the server type. e.g. web server would use server, DB server icon for SQL server/Postgres/etc.

Oooo why can't we just make that change now! I love that idea. I think we already have the needed details to know whether it's a DATA call or otherwise right? Could we do a mock-up of what it would look like if we did what you're proposing?

@JamesNK
Copy link
Member

JamesNK commented Jan 19, 2024

Oooo why can't we just make that change now!

It should be simple to do for the upcoming release. I'd like to get an icon displayed there correctly in this PR. Then we can play with customizing the server icons in a follow up.

I think we already have the needed details to know whether it's a DATA call or otherwise right? Could we do a mock-up of what it would look like if we did what you're proposing?

Yeah. We know HTTP vs database vs redis vs whatever.

Making the change would probably be as easy as doing a mock up.

@adamint
Copy link
Member Author

adamint commented Jan 19, 2024

image

@kvenkatrajan
Copy link
Member

Logged an issue : #1725 for @JamesNK suggestion : #1684 (comment) and approving PR.

@adamint adamint merged commit 835daf9 into dotnet:main Jan 19, 2024
8 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Apr 25, 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.

Make it easier to tell client and server spans apart in tracing
6 participants