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

[docs] ASP.NET Core tracing get started doc #4076

Merged
merged 13 commits into from
Jan 17, 2023

Conversation

CodeBlanch
Copy link
Member

@CodeBlanch CodeBlanch commented Jan 11, 2023

Changes

Received some feedback from @noahfalk that the getting started docs for tracing didn't really point towards the new "StartWithHost" pattern. I tried to fix this up by adding a dedicated ASP.NET Core guide and tweaking the text going over TracerProvider / TracerProviderBuilder to show both patterns.

@CodeBlanch CodeBlanch requested a review from a team January 11, 2023 23:51
@CodeBlanch
Copy link
Member Author

Not sure what I'm going to do about this, but had to laugh 🤣

image

@codecov
Copy link

codecov bot commented Jan 12, 2023

Codecov Report

Merging #4076 (00222ed) into main (ea5a25c) will increase coverage by 0.00%.
The diff coverage is n/a.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #4076   +/-   ##
=======================================
  Coverage   85.61%   85.62%           
=======================================
  Files         289      289           
  Lines       11255    11255           
=======================================
+ Hits         9636     9637    +1     
+ Misses       1619     1618    -1     
Impacted Files Coverage Δ
...p/Implementation/HttpInstrumentationEventSource.cs 72.00% <0.00%> (-4.00%) ⬇️
...emetry.Api/Internal/OpenTelemetryApiEventSource.cs 76.47% <0.00%> (+2.94%) ⬆️
...lementation/SqlClientInstrumentationEventSource.cs 75.00% <0.00%> (+4.16%) ⬆️

@reyang
Copy link
Member

reyang commented Jan 12, 2023

Not sure what I'm going to do about this, but had to laugh 🤣

image

Do we need to put these files in the repo at all? (can they be grabbed at build time)

@cijothomas
Copy link
Member

Notes to be discussed:
Do we need getting started for each type of application? Or we limit to 1. Console 2. Asp.Net Core (this PR) 3. Asp.Net (.NET FW).

.NET's own logging doc splits the logging into 2 - host based vs non-host based, with an entirely separate doc for asp.net core

Create a new web application:

```sh
dotnet new webapp -o aspnetcoreapp
Copy link
Member

Choose a reason for hiding this comment

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

lets do webapi so that the # of files to be checked in are minimal, and also gets rid of the other funny spellcheck thing you mentioned as well!

Copy link
Member Author

Choose a reason for hiding this comment

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

Not opposed to that. FYI the reason I went with the razor web app style is when I searched "aspnetcore get started" I landed on Tutorial: Get started with ASP.NET Core. Since it does have a UI, I also added this:

image

Makes it kind of fun.

But agree it sucks to have all of those files in there. I'm going to mess with it a bit and see if they can be pulled at build time.

```sh
dotnet add package OpenTelemetry.Exporter.Console --prerelease
dotnet add package OpenTelemetry.Extensions.Hosting --prerelease
dotnet add package OpenTelemetry.Instrumentation.AspNetCore --prerelease
Copy link
Member

Choose a reason for hiding this comment

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

I am neutral here... The getting started could be strictly "how to manually create a span in 5 minutes". A "next step" could show the instrumentation library stuff, instead of manual span creation.

Showing instrumentation library here will truly make it a 5 min tutorial and is the likely path a normal user would take.

Will discuss in SIG meetings to get more views on this.

Copy link
Member Author

Choose a reason for hiding this comment

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

Showing instrumentation library here will truly make it a 5 min tutorial and is the likely path a normal user would take.

Sort of what I was thinking. Get up and running quickly, see some data for incoming requests. Manually creating spans could be a next topic/advanced doc for users who are interested 🤷‍♀️

Copy link
Member

@alanwest alanwest Jan 13, 2023

Choose a reason for hiding this comment

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

Manually creating spans could be a next topic/advanced doc

+1. Yes, I've always thought of manually generating telemetry as an advanced topic not a step 1 topic. A lot of folk are coming to OpenTelemetry having used other proprietary products many of which just require a simple install/configuration to see some magic.

@cijothomas
Copy link
Member

https://github.com/open-telemetry/opentelemetry-dotnet/tree/main/examples/Console - This is a prime candidate for nuking. This was meant for developers mainly, not really written with end users in mind. Depending on how this PR goes, we can tweak the examples folder.

@reyang
Copy link
Member

reyang commented Jan 12, 2023

Notes to be discussed: Do we need getting started for each type of application? Or we limit to 1. Console 2. Asp.Net Core (this PR) 3. Asp.Net (.NET FW).

.NET's own logging doc splits the logging into 2 - host based vs non-host based, with an entirely separate doc for asp.net core

I would vote for not covering ASP.NET for now since it is considered "legacy", if it turned out that there is a high demand (I guess very unlikely), we can add it later.

@CodeBlanch
Copy link
Member Author

CodeBlanch commented Jan 12, 2023

Notes to be discussed: Do we need getting started for each type of application? Or we limit to 1. Console 2. Asp.Net Core (this PR) 3. Asp.Net (.NET FW).

.NET's own logging doc splits the logging into 2 - host based vs non-host based, with an entirely separate doc for asp.net core

Ya we could go a lot of ways with it. I think "Host & Non-host" would be good, but might not resonate with users the same way "ASP.NET Core & Console" would. Truth be told the "Console" example is lying because you can make a console app with generic host and use the hosting extensions 😄

@reyang

I would vote for not covering ASP.NET for now since it is considered "legacy", if it turned out that there is a high demand (I guess very unlikely), we can add it later.

Maybe this is where the example apps would be useful. We could just link to them for some other scenarios? For example in contrib we have ASP.NET example.

@TimothyMothra
Copy link
Contributor

TimothyMothra commented Jan 12, 2023

Not sure what I'm going to do about this, but had to laugh 🤣

image

I've had similar issues in other repos. I addressed this by removing all the js and css files.
Since we're only demonstrating .NET best practices, we don't need the entire wwwroot directory.

In my opinion the Error and Privacy pages can also be removed.

@CodeBlanch
Copy link
Member Author

I switched all the npm stuff to use a CDN for retrieval so the artifacts won't be in our repo. The misspell warning should clear but there seems to be an issue with the job at the moment 🤕

Since we're only demonstrating .NET best practices, we don't need the entire wwwroot directory.

I don't feel passionately about this but if we do stick with the razor page style I prefer it to render somewhat nicely & be close to what the user will see with the command line template if they follow the instructions in the README.

@reyang
Copy link
Member

reyang commented Jan 13, 2023

Maybe this is where the example apps would be useful. We could just link to them for some other scenarios? For example in contrib we have ASP.NET example.

+1

Maybe here in the /docs folder we can put something simple, such as https://twitter.com/davidfowl/status/1588942067157594112/photo/1.

For the "more real world examples", combine the effort with opentelemetry-demo project and put the code there?

@CodeBlanch
Copy link
Member Author

Updated to use dotnet new web which generates a minimal style. Thanks to @cijothomas for finding that template 😄

@@ -41,7 +41,8 @@ If you are new here, please read the getting started docs:

* [logs](./docs/logs/getting-started/README.md)
* [metrics](./docs/metrics/getting-started/README.md)
* [traces](./docs/trace/getting-started/README.md)
* traces: [ASP.NET Core](./docs/trace/getting-started-aspnetcore/README.md) |
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, I suggest keep this unchanged, by default users will go to the simple console example (assuming a new user just came and wanted to understand the basics, and the user doesn't have to mix this with the actual integration of OpenTelemetry and an application framework). In the console tutorial doc, I think we can add something in the beginning "this is showing the basics of a simple console application, if you want to learn how to use OpenTelemetry with ASP.NET Core, read X, if you want to learn how to use UWP, read Y, ..."

Copy link
Member Author

Choose a reason for hiding this comment

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

Can we do the reverse? Link to the ASP.NET Core case with a comment for simple console/.NET Framework? I'm assuming more people will be interested in the ASP.NET Core style.

Copy link
Member

Choose a reason for hiding this comment

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

I don't know, here is one thing we need to consider: for folks who want to learn how to extend the SDK or how to configure advanced scenarios, do we want them to get into ASP.NET Core or stay with a basic console app? Or we can ask the question in another way - would C# "Hello, World" choose console or ASP.NET Core?

Copy link
Member

Choose a reason for hiding this comment

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

I guess I'm fine with the "getting started with tracing" doc pointing to ASP.NET Core demo, and in the banner put something like "hey if you don't want to bother with HTTP hosting, here goes a simple console version you want to use" (it seems a bit awkward to me, but I don't see big no).

Copy link
Member

@alanwest alanwest Jan 13, 2023

Choose a reason for hiding this comment

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

for folks who want to learn how to extend the SDK or how to configure advanced scenarios, do we want them to get into ASP.NET Core or stay with a basic console app?

I think this is a good question, and maybe it's true that if a custom SDK extension is your goal then you might want a guide that used a basic console app. Though, I'd say that if you're writing a custom SDK extension, then you're probably already past "getting started" with OTel 😆.

I think our "getting started" users are most likely folks who just want to see some telemetry get emitted from an app, and personally I think the fact we're entangling our getting started guide with an application framework actually serves to better meet this need - i.e., as a new user I don't need to entangle myself with learning anything about the activity api out of the gate.

Choose a reason for hiding this comment

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

Fwiw here is what I did in the .NET docs: https://learn.microsoft.com/en-us/dotnet/core/diagnostics/distributed-tracing-collection-walkthroughs#collect-traces-using-opentelemetry

I did ASP.NET first, console 2nd. My assumption is that ASP.NET is the more common case users will care about and it also has the advantage that you don't need to add any manual instrumentation to see a result for distributed tracing.

Copy link
Member Author

Choose a reason for hiding this comment

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

I chatted with @cijothomas a bit offline. I think ideally we would only have the ASP.NET Core guide. Agree with @noahfalk it seems like what most users will be after and is nice and concise. Manual instrumentation would be a more advanced topic/guide. Console example might be in some other repo. However, because the ASP.NET Core guide currently relies on prerelease packages, and that doesn't seem likely to change anytime soon, we feel like having the console guide close by will be useful for some teams. We may have to live with the two for now. But I did add a banner on the ASP.NET Core guide with a "note" about the prerelease status.

The only open question for me is do we link to both from the main README? Don't have a strong opinion either way.

Copy link
Member

Choose a reason for hiding this comment

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

No strong opinion either (slightly prefer having only one link instead of two, to keep the doc smaller).

Copy link
Member

Choose a reason for hiding this comment

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

Not a strong opinion, but its not just the application type that is changing between 2 tutorials - it also changes manual span creation <-> instrumentation library creation. I do not have a neat alternative suggestion either, lets proceed and keep iterating if fresh ideas come up later.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sounds good, merged.

Copy link
Member

@reyang reyang left a comment

Choose a reason for hiding this comment

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

LGTM.

@CodeBlanch CodeBlanch merged commit bb0c381 into open-telemetry:main Jan 17, 2023
@CodeBlanch CodeBlanch deleted the tracing-docs branch January 17, 2023 21:46
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.

6 participants