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] Add getting started docs showing how to set up OTLP exporter with Aspire dashboard #5779

Closed
wants to merge 12 commits into from

Conversation

CodeBlanch
Copy link
Member

Changes

  • Add getting started docs which show how to use the OtlpExporter with Aspire dashboard

Merge requirement checklist

  • CONTRIBUTING guidelines followed (license requirements, nullable enabled, static analysis, etc.)

@CodeBlanch CodeBlanch requested a review from a team August 8, 2024 22:08
@github-actions github-actions bot added the documentation Documentation related label Aug 8, 2024
Copy link

codecov bot commented Aug 8, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 86.39%. Comparing base (6250307) to head (17d5785).
Report is 290 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #5779      +/-   ##
==========================================
+ Coverage   83.38%   86.39%   +3.00%     
==========================================
  Files         297      256      -41     
  Lines       12531    11140    -1391     
==========================================
- Hits        10449     9624     -825     
+ Misses       2082     1516     -566     
Flag Coverage Δ
unittests ?
unittests-Project-Experimental 86.21% <ø> (?)
unittests-Project-Stable 86.21% <ø> (?)
unittests-Solution 86.38% <ø> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

see 207 files with indirect coverage changes

Copy link

@noahfalk noahfalk left a comment

Choose a reason for hiding this comment

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

Looks pretty good to me, a few comments inline. Were you planning to link anything from the root repo README? I don't know how many people will manually navigate into the docs/otlp sub-folder.


// Set OpenTelemetry metrics to use delta temporality.
builder.Services.Configure<MetricReaderOptions>(
o => o.TemporalityPreference = MetricReaderTemporalityPreference.Delta);
Copy link

Choose a reason for hiding this comment

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

What happens if you don't set this? This feels like a low-level setting that users won't immediately relate to in an app that is otherwise very 'Hello World'.

Copy link
Member

Choose a reason for hiding this comment

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

Does AspireDashboard tool handles delta/cumulative correctly? If it does, then we are good. If it is designed to handle delta only (many backends are!, except prometheus), then this will become inevitable:(

Copy link
Member Author

Choose a reason for hiding this comment

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

Seems to work fine in cumulative with Aspire so I removed it. @JamesNK Can you confirm what is supported?

@noahfalk I agree with you it seems too complex for a "hello world" but I felt best practice it might be best to show it 🤷 Kind of a pit of failure the way the spec has set this part designed IMO. User has to a) understand different modes exist, b) figure out what they need, and c) figure out what the default is and how to change the value if needed. Ouch!

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know. I know Aspire works with metrics + default settings. I haven't tried what happens with delta only.

Choose a reason for hiding this comment

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

If you can avoid having it that seems preferable - it sounds like it is avoidable with Aspire dashboard at least. If you do need it (perhaps you'll want to demo some other OTLP viewers that require it?) then its probably worth some additional explanatory text to help users understand why its there.

Kind of a pit of failure the way the spec has set this part designed IMO

Yeah, that definitely feels unfortunate to have designed the protocol where different tools require different options and there is no auto-negotiation of the setting inside the communication channel.

docs/otlp/getting-started-aspnetcore/Program.cs Outdated Show resolved Hide resolved
docs/otlp/getting-started-aspnetcore/Program.cs Outdated Show resolved Hide resolved
docs/otlp/getting-started-aspnetcore/README.md Outdated Show resolved Hide resolved
started in 5 minutes - Console Application](./getting-started-console/README.md)
guide to get up and running.

## Vendor support
Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Could we just link to website docs, instead of maintaining a list here : https://opentelemetry.io/ecosystem/vendors/

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'm good with either. I think it is nice to link to specific docs for .NET, but I couldn't find one for everything. Also in that huge list not everything supports OTLP ingestion. I updated it to also link there though. LMK

Copy link
Member

Choose a reason for hiding this comment

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

I'd prefer for this repo to not list and maintain this, and instead leverage docs website by linking to it.

If the website listing is not easy/hard-to-filter for .NET etc, then we can address that in the website

@samsp-msft
Copy link

I put a comment inline, but want to re-iterate here, why not use the Aspire template as the starting point. Its has everything already done and working.

The 90% scenario is probably not adding your own Activities and Metrics, but we can walk them through that as stage 2.

it is recommended to first follow the [getting started in 5 minutes - ASP.NET
Core Application](./getting-started-aspnetcore/README.md) guide or the [getting
started in 5 minutes - Console Application](./getting-started-console/README.md)
guide to get up and running.
Copy link
Member

Choose a reason for hiding this comment

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

could we make it clear that the linked tutorials are to get familiariaty with OTel .NET itself, and they export telemetry to console for learning purposes. And this doc shows exporting in the OTel's standard -the OTLP exporter..

Copy link
Member

Choose a reason for hiding this comment

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

After checking existing docs, this feels like we are duplicating a lot of content from getting started.. I don't know the best way to address that yet..

Copy link
Member

@cijothomas cijothomas Aug 13, 2024

Choose a reason for hiding this comment

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

To address my own concern about duplicating the content from getting started:

Wondering if it is better to move this under OTLP Exporter readme instead? It won't be a full blown example (with .csproj etc.), but .md file which shows the expected content of program.cs and .csproj along with instruction to use Aspire to view the telemetry. The current one points to example in this repo, which is setting up otel collector, so this can be a good replacement.

https://github.com/open-telemetry/opentelemetry-dotnet/tree/main/src/OpenTelemetry.Exporter.OpenTelemetryProtocol#enable-otlp-exporter-for-all-signals

@martinjt
Copy link
Member

martinjt commented Aug 9, 2024

I'm not sure why we're putting docs here, where's the request coming from? The Docs need to live in open-telemetry/docs, these examples are legacy ones. If this is something we think is valuable then it should be debated in an OpenTelemetry docs PR.

I also don't think we should be talking about Aspire here. Aspire has it's own docs on the Microsoft site. We definitely shouldn't be changing the template to use the Aspire one, that's the beyond the scope of the OpenTelemetry project.

@noahfalk
Copy link

noahfalk commented Aug 9, 2024

I also don't think we should be talking about Aspire here

I didn't think the intent was to be a doc on how to use Aspire. I believe @CodeBlanch chose the dashboard because its a free + OSS viewer for OTLP data that is trivial to set up. The dashboard is a standalone tool that can run completely independently of Aspire and consume some arbitrary OTLP telemetry. For example here is a python telemetry demo that felt the dashboard was a good choice https://tonybaloney.github.io/posts/using-dotnet-aspire-dashboard-for-python-opentelemetry.html.

@martinjt
Copy link
Member

martinjt commented Aug 9, 2024

Thanks @noahfalk, I'm fully aware of the dashboard and it's capabilities, I do advocate for it. This just isn't the place to promote it.

Like I said, these examples are legacy, and need to be all moved into the main docs. If there's a tool that people should be using, it should be flagged across all the languages not just .NET and that's a much wider discussion that should be raised in the docs project.

using Docker:

```sh
docker run --rm -it -p 18888:18888 -p 4317:18889 -p 4318:18890 -d --name aspire-dashboard mcr.microsoft.com/dotnet/aspire-dashboard:latest
Copy link
Contributor

Choose a reason for hiding this comment

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

When I follow these instructions and then browse to the .NET Aspire
dashboard (eg http://localhost:18888/) to view my telemetry, I get:

image

I know how to get around this, but I'm not sure someone new would be able to figure it out. I think we should either:

  1. Set the switch to disable the log in.
    -OR-
  2. Describe what someone needs to do to log in to the dashboard

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 updated the guides to disable auth. Also added a "caution" note about it. LMK


public static void Main()
{
// Note: OpenTelemetrySdk.Create was added in 1.10.0
Copy link
Contributor

Choose a reason for hiding this comment

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

This won't work for people until they can get access to 1.10.0. When will that ship?

Copy link
Member

Choose a reason for hiding this comment

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

Nov this year.

Copy link
Member Author

Choose a reason for hiding this comment

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

We could put out an alpha if you think that would help? I would prefer to not use the new style yet in guides but you can't use the cross-cutting "UseOtlpExporter" call with the old style. I felt that was more important for these particular guides.

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess it gets to the question of "who do you expect to read/follow these instructions between now and when 1.10.0 ships?"

If it is someone who you expect to know how to get this code to work (which excludes me), then the current form is fine.

But if it is someone who is just getting started here, then maybe we need to update the instructions to tell them how to make this app work. Or change the app to make it just work for right now. As it stands, I don't believe a new person would be successful trying to follow these instructions since this code doesn't compile.

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 put some stronger notices in there that it won't work/is a work in progress. But I'm hoping it is short lived and I can get it working as soon as we drop an alpha/beta.

@cijothomas
Copy link
Member

cijothomas commented Aug 9, 2024

I put a comment inline, but want to re-iterate here, why not use the Aspire template as the starting point. Its has everything already done and working.

This is just meant to show OpenTelemetry usage, and nothing more.

edit: +1 to below.

We definitely shouldn't be changing the template to use the Aspire one, that's the beyond the scope of the OpenTelemetry project.

@martinjt
Copy link
Member

martinjt commented Aug 9, 2024

I think we need answers to the following questions:

  1. Where is the request coming from to add this content? and why is there a need for it?
  2. Why is this example in the .NET OpenTelemetry repo and not the Microsoft Aspire site?
  3. Why is this in the repo, and not on the docs site?
  4. Why should OpenTelemetry be promoting a thirdparty, non-CNCF tool?

I know what the Microsoft Aspire dashboard is, and why it's useful, but this isn't something that OpenTelemetry has adopted. If this is something we're doing now, we should look at adding examples of the signoz, qryn, grafana stacks too, but that's then a very slippery slope.

There is an ongoing discussion about a local visualisation tool, and until that's resolved, I don't think this should be part of the official repo.

@CodeBlanch
Copy link
Member Author

I think we need answers to the following questions:

  1. Where is the request coming from to add this content? and why is there a need for it?
  2. Why is this example in the .NET OpenTelemetry repo and not the Microsoft Aspire site?
  3. Why is this in the repo, and not on the docs site?
  4. Why should OpenTelemetry be promoting a thirdparty, non-CNCF tool?

I know what the Microsoft Aspire dashboard is, and why it's useful, but this isn't something that OpenTelemetry has adopted. If this is something we're doing now, we should look at adding examples of the signoz, qryn, grafana stacks too, but that's then a very slippery slope.

There is an ongoing discussion about a local visualisation tool, and until that's resolved, I don't think this should be part of the official repo.

Here are my replies..

  1. Where is the request coming from to add this content? and why is there a need for it?

No one asked for it 🤣 I needed to test OTLP transmission for some code I was working on. I tried the Aspire dashboard standalone container and it worked great. We have one-off guides. A Jaeger one. A Prometheus+Grafana one. Nothing for logs, I don't think. I thought it would be great to show all signals together using OTLP and Aspire made it really easy to do that.

  1. Why is this example in the .NET OpenTelemetry repo and not the Microsoft Aspire site?

This isn't an Aspire thing. This is an OTLP thing. Just happens to use Aspire for the visualization. Happy to swap out Aspire if there is something that works equally well with relative ease.

  1. Why is this in the repo, and not on the docs site?

No real reason other than I just took the existing getting started docs in the repo and modified them for OTLP. Status quo more or less.

  1. Why should OpenTelemetry be promoting a thirdparty, non-CNCF tool?

It certainly wasn't the intention to promote Aspire. The goal is to promote OTLP! What I wanted to show was how easy it is (few lines of code) to enable all signals with OTLP export. Also how easy it is for users to send them wherever they want. Having the visualization is just a nice-to-have for new users to immediately see the fruits of their labor and get excited about OpenTelemetry.

If this is something we're doing now, we should look at adding examples of the signoz, qryn, grafana stacks too, but that's then a very slippery slope.

We do promote other OSS stacks though. The only thing we didn't promote was OTLP which is really the de facto way to do OpenTelemetry in my mind. The goal with this guide is for us to say to users OTLP is what you should be doing, not the other stuff 😄

@samsp-msft
Copy link

  1. Why should OpenTelemetry be promoting a thirdparty, non-CNCF tool?

It certainly wasn't the intention to promote Aspire. The goal is to promote OTLP! What I wanted to show was how easy it is (few lines of code) to enable all signals with OTLP export. Also how easy it is for users to send them wherever they want. Having the visualization is just a nice-to-have for new users to immediately see the fruits of their labor and get excited about OpenTelemetry.

As somebody who bridges between Aspire and OTel, I don't see how this is promoting Aspire, it's using the standalone dashboard to provide a quick and easy way to view OTLP data. I have written docs on how to send telemetry to Prometheus/Grafana/Yeager. The steps required to configure all of them dwarf the topics of how to setup telemetry - the standalone dashboard illustrates that concept with one call to docker, and one line setting up the OTLP exporter in the app. There is no use of Aspire libraries or other changes to the project that would tie the projects to Aspire.

The work we are doing in Aspire is to make using OpenTelemetry easy. There is nothing proprietary in Aspire when it comes to telemetry - its providing (hopefully) good defaults for setting up and configuring OTel in your projects. We include the OTLP exporter, both to drive the dashboard and because we see it as setting up the apps for the long term for telemetry. Aspire is not tied to Microsoft/Azure backends.

@martinjt
Copy link
Member

I've discussed this with a lot of people, and the general consensus is that unless this is something that would be accepted by docs, it shouldn't be merged here. This repo isn't the place for documentation or guides, that's been debated a lot and the docs team have been clear that docs is the place for this.

If there is an example application, that details how to run the example, that's different, but this is clearly a guide, with narrative on how and when to use it. This is evidenced by the title, and the code in the readme in telling people how to recreate it in code.

My suggestion is to look at how this could be formatted into something that could go into the main docs site, or as a blog for the OpenTelemetry blog. However, it's not something that should live in this repo. If people believe that Aspire is truly useful and the one solution here, you shouldn't have a problem justifying that with the docs team as the solution they use for all languages.

If we wanted to make this in-keeping with the other example solutions, all the narrative should be removed from the readme and instead it should say how to run the solution only.

@cartermp
Copy link
Contributor

Speaking on behalf of the docs side of things, I wouldn't say no to an Aspire-related "get started" or some other document on the website, especially if we expect many .NET developers who learning OTel and Observability to use Aspire. I don't view it as terribly different from ASP.NET Core in that regard.

@cijothomas
Copy link
Member

Speaking on behalf of the docs side of things, I wouldn't say no to an Aspire-related "get started" or some other document on the website, especially if we expect many .NET developers who learning OTel and Observability to use Aspire. I don't view it as terribly different from ASP.NET Core in that regard.

Just to be clear, Aspire (the use case shown here) has nothing to do with .NET. Its just used as a tool to view all 3 signals with a one line exe/docker. If there is another tool blessed by spec/docs, we should use that. If there is none today, the SIG can chose aspire, and once spec/doc recommends a particular tool, this should be replaced with the recommendation.

@martinjt
Copy link
Member

@cijothomas that doesn't change fact that this doesn't live in this repo though. As I said you should take that to the docs team as a PR, and it should be removed from here.

On "it has nothing to do with .NET", I think that's a little disingenuous. It's written by the team who works on .NET, written in .NET, and has primarily targeted .NET developers for the last 12 months and only recently has it branched out to other languages since the dashboard has been launched as a separate thing. It has a lot to do with .NET. I agree that the dashboard can (and has) been used outside of pure .NET solutions, but to say it has nothing to do with .NET is not accurate.

@mtwo
Copy link
Member

mtwo commented Aug 13, 2024

Following up from the SIG call and from discussions with the GC and @svrnm (should have also discussed with you @cartermp, apologies): there's a longstanding desire to add getting started guides to the opentelemetry.io website, and this would be a great example of this. Aspire is OSS and permissively licensed, so it's a great fit for this.

I've messaged @svrnm, and he or one of the other docs contributors will likely reach out to help move this over if we're all agreed.

@cijothomas I didn't catch you at the SIG, but I know that you're active in the comments. Are you okay with this plan?

Copy link
Member

@alanwest alanwest left a comment

Choose a reason for hiding this comment

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

Based on the discussion in the SIG, I agree this would be a great fit for opentelemetry.io rather than this repo.

I really like that the Aspire dashboard offers a means for visualizing data easily in the setting of a getting started guide. In fact maybe it could be the tool used for getting started guides in other languages.

As this PR currently stands, it demonstrates some APIs that have not been released stable yet. It probably makes sense to move this work to opentelemetry.io and use only stable APIs for now. Come November when 1.10 is released the getting started guide could be updated to reflect the new APIs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Documentation related
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants