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

Recreate examples #1708

Merged
merged 32 commits into from
Dec 7, 2022
Merged

Recreate examples #1708

merged 32 commits into from
Dec 7, 2022

Conversation

pellared
Copy link
Member

@pellared pellared commented Dec 5, 2022

Why

Fixes #1388

What

Create new example which should be more user-friendly. It works for the latest "non-beta" release.

Check examples/README.md.

I also consider to try adding Grafana Loki as a separate PR.

Tests

Automated test via make all and a GitHub workflow to test it in CI.

See: https://github.com/open-telemetry/opentelemetry-dotnet-instrumentation/actions/runs/3624194494/jobs/6110899177

Checklist

  • CHANGELOG.md is updated.
  • Documentation is updated.
  • New features are covered by tests.

@github-actions github-actions bot requested a review from theletterf December 5, 2022 14:33
@pellared pellared marked this pull request as ready for review December 5, 2022 22:55
@pellared pellared requested a review from a team December 5, 2022 22:55
Copy link
Contributor

@pjanotti pjanotti left a comment

Choose a reason for hiding this comment

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

LGTM. Just minor Qs and suggestions.

.github/dependabot.yml Outdated Show resolved Hide resolved
docs/troubleshooting.md Outdated Show resolved Hide resolved
examples/Makefile Show resolved Hide resolved
examples/README.md Outdated Show resolved Hide resolved
examples/README.md Outdated Show resolved Hide resolved
.github/dependabot.yml Outdated Show resolved Hide resolved
@pellared pellared requested review from Kielek and pjanotti December 6, 2022 12:29
@rajkumar-rangaraj
Copy link
Contributor

I agree that the proposed changes makes it simpler for users to execute and instrument the sample app.
But, I strongly feel we should have one of ASP.NET Core based project as a part of the example. It will help us debug and validate our changes. Shall we keep current AspNetCoreMvc project along with proposed changes in this PR?

@pellared
Copy link
Member Author

pellared commented Dec 6, 2022

I agree that the proposed changes makes it simpler for users to execute and instrument the sample app. But, I strongly feel we should have one of ASP.NET Core based project as a part of the example. It will help us debug and validate our changes. Shall we keep current AspNetCoreMvc project along with proposed changes in this PR?

@rajkumar-rangaraj Server is an ASP.NET Core app (minimal Web API). Do you prefer to change it to use the ASP.NET Core Web Host? We can also add another HTTP service and they communicate using MassTransit. Do you have any preference?

Personally, I would lean towards refactoring the existing Service application.

We may also merge this PR as it is and you can create a refactor PR if you find it easier.

It will help us debug and validate our changes

I would rather use test applications for it 😉

docs/releasing.md Outdated Show resolved Hide resolved
Copy link
Contributor

@zacharycmontoya zacharycmontoya left a comment

Choose a reason for hiding this comment

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

I glossed over the yml files for the docker-compose and all the config, but everything else looks good to me 👍🏼

Co-authored-by: Zach Montoya <zach.montoya@datadoghq.com>
@pellared pellared merged commit bac9c02 into open-telemetry:main Dec 7, 2022
@pellared pellared deleted the new-examples branch December 7, 2022 07:22
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.

Recreate /examples
5 participants