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

Fix inconsistent trace detail spacing #1863

Merged
merged 2 commits into from
Jan 26, 2024
Merged

Conversation

JamesNK
Copy link
Member

@JamesNK JamesNK commented Jan 26, 2024

Adding an icon creating inconsistent spacing. There isn't a way to perfectly balance everything (the current color bar for client spans is thinner than icons) but this is the best layout I could find.

Before:

image

After:

image

Microsoft Reviewers: Open in CodeFlow

@tlmii
Copy link
Member

tlmii commented Jan 26, 2024

I'm a little worried about all the magic numbers here (we haven't been great about it in other places, but I ran into it during the design changes work and realized I should bring it up). The below write-up is for anyone not familiar with the design unit work, not so much the author here.

Generally with fast/fluent you want to base things on the --design-unit and --density css custom properties so that you can tweak those and everything shifts proportionally. The default design unit (and what we use) is 4 (no units) and the default density is 0 (and usually just in a range of -2 to 2). A fairly common example would be to do calc((var(--design-unit) + var(--density)) * 1px). At the default values that'd be 4px. But we could say "hey this control needs to more/less dense" and just tweak the density value +/- a couple and the whole layout would expand/shrink accordingly. Or we could decide that for mobile or whole site needed to have elements that are slightly larger so we should bump the design unit up to 5 or 6 to be more touch-friendly.

Strictly as an example (I haven't looked at what this actually looks like when running, it wouldn't match what you have pixel-for-pixel; and this example only uses the design unit to keep it somewhat brief):

var isServerKind = context.Span.Kind == OtlpSpanKind.Server;
// Indent the span name based on the depth of the span.
var baseDepth = (context.Depth - 1);

// We want to have consistent margin for both client and server spans.
string spanNameContainerStyle;
if (!isServerKind)
{
    // Client span has 19px extra content:
    // - 1 design unit border
    // - 2 design unit padding-left
    // - 1 design unit extra margin-left
    spanNameContainerStyle = $"--base-depth: {baseDepth}; --extraDepth: 1; margin-left: calc((var(--design-unit) * var(--baseDepth) * 4px) + (var(--design-unit) * var(--extraDepth) * 1px)); border-left-color: {ColorGenerator.Instance.GetColorHexByKey(GetResourceName(context.Span.Source))}; border-left-width: calc(var(--design-unit) * 1px); border-left-style: solid; padding-left: calc(var(--design-unit) * 2px);";
}
else
{
    // Server span has 19px extra content:
    // - 16px icon
    // - 3px padding-left
    spanNameContainerStyle = $"--base-depth: {baseDepth}; --extraDepth: 0; margin-left: calc((var(--design-unit) * var(--baseDepth) * 4px) + (var(--design-unit) * var(--extraDepth) * 1px));";
}

You could also then, possibly move the margin and everything after it into a stylesheet and just have the --base-depth and --extra-depth be inline. That'd keep the style part that has to be in C#/razor code a little more clean.

Copy link
Member

@kvenkatrajan kvenkatrajan left a comment

Choose a reason for hiding this comment

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

Approved- please file follow up issue for spacing with design units.

@joperezr
Copy link
Member

joperezr commented Jan 26, 2024

Discussed this offline with @kvenkatrajan and @tlmii , and we agreed to take this for preview 3 and to log an issue regarding the concern raised by @tlmii above.

@joperezr joperezr merged commit 6fe5242 into main Jan 26, 2024
8 checks passed
@joperezr joperezr deleted the jamesnk/trace-detail-spacing branch January 26, 2024 23:46
joperezr added a commit that referenced this pull request Feb 1, 2024
* Change default port for dashboard applicationUrl (#1853)

Windows allows systems to exclude specific port ranges, after which attempts to bind them will fail. The ranges will vary from machine to machine, and can be viewed with the command `netsh interface ipv4 show excludedportrange protocol=tcp`.

We're seeing users hitting issues with port 63824. While the excluded ranges will vary from machine to machine, it seems like these higher numbers are more likely to be excluded.

This change moves the ports lower, which should help alleviate this issue.

* Add details column to trace view, remove status badge and put counter badge on resource name (#1788)

* add details column to trace detail

* Update xlfs, style button

* remove error badge

* Put unread error counter on name

* update trace detail details column to absolute width

* Remove unused resx strings, re-add titles for resource name

* Make error counter badge text white in dark theme

* fixed variable name

---------

Co-authored-by: Adam Ratzman <adamratzman@microsoft.com>

* Fix horizontal overflow in metrics popup (#1838)

* Remove unused "known property" getter methods (#1856)

This code is no longer used, so remove it.

* Update dashboard README (#1854)

* Fix trace detail page's gap between trace ticks (#1861)

* Fix SQL server resource version (#1870)

* Add Pomelo.EntityFrameworkCore.MySql component (#1161)

* Add Pomelo.EntityFrameworkCore.MySql component.

* Add Pomelo.EntityFrameworkCore.MySql tests.

* Update XML documentation.
* Update XML documentation to current conventions.
* Add documentation for Pomelo metrics.

* Use configuration schema generator.

* Allow ServerVersion setting to be optional.

* Remove (now optional) ServerVersion setting from test.

* Remove MySqlConnector logging categories from Pomelo section.

* Add Microsoft.Extensions.Configuration.Binder.

* Add basic MySQL Aspire.Hosting tests.

These were modeled on AddPostgresTests and AddRedisTests.

* Add Pomelo EFCore integration tests.

* Incorporate project change from dff1467.

* PR feedback

- Add log categories to ConfigurationSchema
- Check the "API" boxes for the MySql components in the Progress doc
- Issue a query in the functional test. Follow the same pattern as Oracle
- Set the ServerVersion explicitly in the functional test, so the connection can be retried when the MySql server isn't running.

---------

Co-authored-by: Eric Erhardt <eric.erhardt@microsoft.com>

* Fix TryAddWillNotAddTheSameLifecycleHook test. (#1862)

* Use the right health check properties in ServiceBus README. (#1876)

Fix #1715

* Trace detail page - customize peer icons based on span attributes (#1865)

* Update SqlServerBuilderExtensions.cs (#1883)

Follow up to #1870

* Fix Storage Queue and Cosmos EF READMEs (#1884)

* Fix Storage Queue README

The ConnectionStrings setting had the wrong name, so it doesn't work with what is in the code section above it.

Fix #1679

* Fix CosmosDB README

The extension method takes 2 strings: connection name and database name.

Fix #873

* remove link in name column (#1886)

Co-authored-by: Adam Ratzman <adamratzman@microsoft.com>

* Orleans: enable distributed tracing by default (#1858)

* add failedtostart error icon to state (#1887)

* add failedtostart error icon to state

* add starting icon

---------

Co-authored-by: Adam Ratzman <adamratzman@microsoft.com>

* Fix grid layout issue (#1889)

* Update dependencies from https://github.com/microsoft/usvc-apiserver build 0.1.49+5 (#1890)

Microsoft.DeveloperControlPlane.darwin-amd64 , Microsoft.DeveloperControlPlane.darwin-arm64 , Microsoft.DeveloperControlPlane.linux-amd64 , Microsoft.DeveloperControlPlane.linux-arm64 , Microsoft.DeveloperControlPlane.windows-386 , Microsoft.DeveloperControlPlane.windows-amd64 , Microsoft.DeveloperControlPlane.windows-arm64
 From Version 0.1.48 -> To Version 0.1.49

Co-authored-by: dotnet-maestro[bot] <dotnet-maestro[bot]@users.noreply.github.com>

* Fix inconsistent trace detail spacing (#1863)

* Fix inconsistent trace detail spacing

* Clean up

* Playground app for SQL Server resources (uses EF). (#1895)

* End to end SQL playground.

* Branding changes for preview 4 (#1892)

* Fix dashboard not working with two applicationUrls specified. (#1901)

* Fix dashboard not working with two applicationUrls specified.

* PR feedback. Defensive code.

* Remove unnecessary code from AppHost in Orleans playground  (#1904)

* remove unnecessary code from Orleans AppHost

* format comment in Orleans AppHost

* Added a mongo playground project (#1921)

* Consistent resource type summaries (#1772)

* Removed static web assets from aspire hosting (#1923)

- We're no longer embedding the dashboard so this shouldn't be required.

* Transform outgoing address to try to find a resource match (#1932)

* Improve environment variable debugging in the app model (#1746)

* Move shared code to shared directory (#1751)

* Fixed build from shared code change (#1937)

* Allow collapsing operations in the trace view (#1323)

Co-authored-by: James Newton-King <james@newtonking.com>

* Update Extensions to preview 1 versions (#1891)

* Update dependencies from https://github.com/dotnet/arcade build 20240125.5 (#1947)

Microsoft.DotNet.Arcade.Sdk , Microsoft.DotNet.Build.Tasks.Installers , Microsoft.DotNet.Build.Tasks.Workloads , Microsoft.DotNet.Helix.Sdk , Microsoft.DotNet.RemoteExecutor , Microsoft.DotNet.SharedFramework.Sdk , Microsoft.DotNet.XUnitExtensions
 From Version 8.0.0-beta.24060.4 -> To Version 8.0.0-beta.24075.5

Co-authored-by: dotnet-maestro[bot] <dotnet-maestro[bot]@users.noreply.github.com>

* Update dependencies from https://github.com/dotnet/extensions build 20240126.7 (#1954)

Microsoft.Extensions.Http.Resilience
 From Version 9.0.0-alpha.1.24076.5 -> To Version 9.0.0-preview.1.24076.7

Co-authored-by: dotnet-maestro[bot] <dotnet-maestro[bot]@users.noreply.github.com>

* Fix dotnet watch. (#1933)

* Fix dotnet watch.

* Fix precondition in DcpHostService (#1943)

* Fix precondition in DcpHostService

Two lines are changed here, though it's the second (in `StopAsync`) that has a functional change.

The first guard clause allows a null publisher, or a "dcp" publisher.

The second clause would allow any non-null publisher, which looks like a bug to me. Specifically, the first half of the || excludes "dcp", then the second half allows it back in. This means that there's no point checking for "dcp" in the code as written.

My interpretation of this code is that both clauses are intended to be the same, and this commit brings them in line with one another.

* Deduplicate condition

* Fix histogram chart calculation (#1968)

Co-authored-by: Drew Noakes <git@drewnoakes.com>

* Fix exception when app host has no ASPNETCORE_URLS (#1970)

If an app host project's launch profile doesn't specify a `launchUrl` or `applicationUrl` then the running app host won't have a `ASPNETCORE_URLS` environment variable. We want to pass this variable's value to the dashboard process, and currently if it's missing we throw.

The dashboard executable has a fallback value when the `ASPNETCORE_URLS` variable is not present (of `http://localhost:18888`), so it's perfectly happy to run without this variable.

This change causes the app host to fall back to that same default value for the dashboard's `ASPNETCORE_URLS` variable. While we could pass no value at all, and have the dashboard choose the default, it's more helpful to the user if we log the dashboard URL. This allows the user to find the dashboard, as the IDE will not be launching it automatically when in this state.

* Revert state column badge changes (#1955)

* revert state column badge changes

* Slightly darken error in dark theme

* Remove spurious VS-added itemgroups

* Auto-close empty element

---------

Co-authored-by: Adam Ratzman <adamratzman@microsoft.com>

* Update OpenTelemetry packages and use built-in metric methods (#1948)

* Update OpenTelemetry packages

- Update all the OpenTelemetry packages to their latest versions.
- Use built-in methods to add .NET metrics.
- Add suppression for new AoT warning on `AddSqlClientInstrumentation()`.

* Resolve review feedback

Mark as not AoT compatible due to dependency on OpenTelemetry.Instrumentation.SqlClient, which itself is not AoT compatible.

* Prevent ANE when resource state is null (#1975)

Ideally we'd make the field nullable on the proto definition, but that's a breaking change at this point. So instead, we pass an empty string.

Other options are discussed in #1930.

* Remove PublishBuildArtifacts (#1990)

This already happens as part of the publish step in the build. Doing this over again (especially to the same container) will overwrite the previous results in some cases, and also unnecessarily duplicate files. It also will cause the wixpacks to get picked up for signing validation.

* Updating some new projects to target net9.0

* Skip Pomelo tests failing due to Pomelo depending on EF 8.0 methods that were removed in 9.0

---------

Co-authored-by: Drew Noakes <git@drewnoakes.com>
Co-authored-by: Adam Ratzman <adam@adamratzman.com>
Co-authored-by: Adam Ratzman <adamratzman@microsoft.com>
Co-authored-by: James Newton-King <james@newtonking.com>
Co-authored-by: Mitch Denny <midenn@microsoft.com>
Co-authored-by: Bradley Grainger <bgrainger@gmail.com>
Co-authored-by: Eric Erhardt <eric.erhardt@microsoft.com>
Co-authored-by: David Fowler <davidfowl@gmail.com>
Co-authored-by: Reuben Bond <203839+ReubenBond@users.noreply.github.com>
Co-authored-by: Tim Mulholland <tlmii@users.noreply.github.com>
Co-authored-by: dotnet-maestro[bot] <42748379+dotnet-maestro[bot]@users.noreply.github.com>
Co-authored-by: dotnet-maestro[bot] <dotnet-maestro[bot]@users.noreply.github.com>
Co-authored-by: Jose Perez Rodriguez <joperezr@microsoft.com>
Co-authored-by: Vincent Hoogendoorn <dev@vincenth.net>
Co-authored-by: Stephan Bauer <67107950+stbau04@users.noreply.github.com>
Co-authored-by: Martin Costello <martin@martincostello.com>
Co-authored-by: Matt Mitchell <mmitche@microsoft.com>
@github-actions github-actions bot locked and limited conversation to collaborators Apr 24, 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.

4 participants