dotnet / efcore

EF Core is a modern object-database mapper for .NET. It supports LINQ queries, change tracking, updates, and schema migrations.
https://docs.microsoft.com/ef/
MIT License
13.74k stars 3.18k forks source link

Move to embedded symbols #15316

Closed SimonCropp closed 2 years ago

SimonCropp commented 5 years ago

From my observations, it seems embedded symbols are becoming the recommended default

Any thoughts on taking this approach for the EF libs?

roji commented 5 years ago

@SimonCropp embedded PDBs (as opposed to portable PDBs and snupkg) don't seem to be a consensus yet (e.g. due to the 20-30% increase in assembly size). See also the conversation in https://github.com/aspnet/Universe/issues/131 and specifically the decision to keep symbols on the MS symbol server (although it's true that it's a year later and decisions can change).

ajcvickers commented 5 years ago

@Eilon @natemcmaster What is the rest of ASP.NET doing here?

natemcmaster commented 5 years ago

We are not embedding symbols. We publish ASP.NET and EF symbols to the Microsoft Symbols Server.

Eilon commented 5 years ago

I think SNUPKG is the eventual goal, but for now everything goes to the MS Symbol Server.

Embedded PDBs (in various forms) are not in the plans for the reasons @roji mentioned.

roji commented 5 years ago

Am closing this as answers have been provided above (but of course we can continue discussing if you want @SimonCropp).

SimonCropp commented 5 years ago

thanks for the consideration.

IMHO the size difference shouldnt come into the discussion. Since we always want line numbers when an exception occurs, people should always deploy pdbs. Also given EF is primarily deployed to servers, the size cost should be even less relevant.

But as i said this is IMO, so happy with the current decision.

roji commented 5 years ago

Since we always want line numbers when an exception occurs, people should always deploy pdbs.

As a general rule, getting line numbers works without necessarily having the PDBs inside the assemblies - or even next to them - the symbol server (and new snupkg mechanism) should provide that.

SimonCropp commented 5 years ago

symbol server and new snupkg do not wokr by default and require people to know both that they can be enabled and that it will enable line lumbers in production. Many .net devs i have worked with in enterprise orgs do not even know what a symbol server is, and none of those orgs have had them enabled. Anything that "phones home" needs to go through the security team. And when the choice comes down to adding 20-30% size to a deployment size (usually in the order of 10s of MB), the option is always to just use the disk space.

Also looping back to the 20-30% size increase. That is a little misleading. it is 20-30% of all IL in an assembly. it is not 20-30% of any resources or large strings in an assembly. Also when taken in the context of a deployed app, the % increase is usually insignificant. for example take a web site deployment as an example. it has images, html, js, etc. "also deploying pdbs" results in a closer to 5% overall increase. And again we are in the order of 10s of MBs here

SimonCropp commented 5 years ago

As a real world indicator. I looked through 13 recent issues with stack traces and only one had line numbers for EF. https://github.com/aspnet/EntityFrameworkCore/issues/14913

The others

roji commented 5 years ago

@SimonCropp I was under the impression that the Microsoft symbol server just works out of the box, without any specific configuration - but I haven't tested this and could be wrong. If there are issues with symbol server and/or the new snupkg, it seems more correct to fix those issues at the source rather to try to work around them by including PDBs everywhere.

As to whether the extra space is too much or not, that's not really for me to say - although it does add up quickly. In any case, this issue is much more general than just EF Core, and a uniform decision is made at a higher level for related projects...

SimonCropp commented 5 years ago

seems more correct to fix those issues at the source rather to try to work around them by including PDBs everywhere.

as long as your solution is to use the internet from servers, u will not be able to fix it for a large portion of users

poke commented 5 years ago

I think what we just need is a simple way to load symbols from a symbol server ahead of time, for example with a separate nuget command as @tmat suggested in that other linked issue.

Changing the default to something inferior when we are actually moving to a more flexible future (with snupkg and a NuGet symbol server that is configured by default) would be the wrong move here.

Many .net devs i have worked with in enterprise orgs do not even know what a symbol server is, and none of those orgs have had them enabled.

Chances are, that they also have “Just My Code” enabled as well then.

I think it would be better to educate those about the possibilities and generally improve documentation and guidance on these topics, instead of forcing everyone to go the same route when it has actual drawbacks and does not fit everyone’s use case.

SimonCropp commented 5 years ago

@poke i am referring to exceptions that occur in production. so “Just My Code” is not relevant. And as i mentioned, usually production servers will not have access to download from a symbol server or

Changing the default to something inferior when we are actually moving to a more flexible future (with snupkg and a NuGet symbol server that is configured by default) would be the wrong move here.

Seems we a moving to a complex future with more moving pieces

I think it would be better to educate those about the possibilities and generally improve documentation and guidance on these topics

so your recommendation would be to tell people to have up the firewall so production servers can access a symbol server?

SimonCropp commented 5 years ago

The solutions of symbols servers and symbols packages where formed at a time when a pdb was 100-200% the size of the assembly, and at a time when file storage and bandwidth was more constrained. Now symbols are 20-30% increase in size and storage and bandwith is several factors higher, i think it is worthwhile re-evaluating if the current solutions (and associated architectural complexity) are correct.

roji commented 5 years ago

The solutions of symbols servers and symbols packages where formed at a time when a pdb was 100-200% the size of the assembly, and at a time when file storage and bandwidth was more constrained. Now symbols are 20-30% increase in size and storage and bandwith is several factors higher, i think it is worthwhile re-evaluating if the current solutions (and associated architectural complexity) are correct.

@SimonCropp you should probably read this comment linked to by @Poke above.

Aside from that, regardless of what I personally think, the EF Core repo isn't the right place for this kind of general discussion - the people who know all the details and who are involved in the actual decision making aren't present here. Consider posting in https://github.com/dotnet/sdk/issues/2679.

tmat commented 5 years ago

am referring to exceptions that occur in production.

If this is about having line information in stack traces in production then the deployment process of the application needs to include the PDBs with the application (next to the corresponding dll). Symbol servers won't work in that case, since the CLR stack trace processing code is only looking on the file system.

The application binaries can embed their PDBs to the assemblies, or deploy them in separate files next to the dlls.

We do not want PDBs to be embedded in framework (runtime) assemblies since then everyone would have to pay for the extra size even if they never used the PDBs. If the application wants to have line numbers for stack frames in framework assemblies it can deploy the PDBs. I don't think we have good tooling that would help with that currently, but it's something we are looking into.

SimonCropp commented 5 years ago

. Symbol servers won't work in that case, since the CLR stack trace processing code is only looking on the file system.

@tmat thanks for the clarification. I did not know that