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

Update .NET manual instrumentation docs with example app #3834

Merged
merged 28 commits into from
May 16, 2024

Conversation

IAMebonyhope
Copy link
Contributor

@IAMebonyhope IAMebonyhope commented Jan 20, 2024

This is to solve the .Net aspect of this issue #3229
Completed the Example App, Setup and Traces section.

I can add metrics and logs in another PR.

Copy link

linux-foundation-easycla bot commented Jan 20, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

@svrnm
Copy link
Member

svrnm commented Jan 20, 2024

thanks for kicking this off @IAMebonyhope !

Here's the preview: https://deploy-preview-3834--opentelemetry.netlify.app/docs/languages/net/instrumentation/

cc @open-telemetry/docs-approvers , @open-telemetry/dotnet-approvers

Copy link
Member

@theletterf theletterf left a comment

Choose a reason for hiding this comment

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

Thanks! Some language and formatting suggestions.

content/en/docs/languages/net/instrumentation.md Outdated Show resolved Hide resolved
content/en/docs/languages/net/instrumentation.md Outdated Show resolved Hide resolved
content/en/docs/languages/net/instrumentation.md Outdated Show resolved Hide resolved
content/en/docs/languages/net/instrumentation.md Outdated Show resolved Hide resolved
content/en/docs/languages/net/instrumentation.md Outdated Show resolved Hide resolved
content/en/docs/languages/net/instrumentation.md Outdated Show resolved Hide resolved
content/en/docs/languages/net/instrumentation.md Outdated Show resolved Hide resolved
@IAMebonyhope IAMebonyhope marked this pull request as ready for review February 9, 2024 00:06
@IAMebonyhope IAMebonyhope requested review from a team February 9, 2024 00:06
Copy link
Member

@martinjt martinjt left a comment

Choose a reason for hiding this comment

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

It's definitely looking good @IAMebonyhope! Added some comments, feel free to tag me if you have more!

content/en/docs/languages/net/instrumentation.md Outdated Show resolved Hide resolved
@svrnm
Copy link
Member

svrnm commented May 2, 2024

@IAMebonyhope anything you need to move this forward?

@IAMebonyhope
Copy link
Contributor Author

@IAMebonyhope anything you need to move this forward?

@svrnm I'll work on resolving the comments this week

@IAMebonyhope
Copy link
Contributor Author

Pushed a new iteration and responded to comments @svrnm

@cijothomas @reyang please can you take another look?

//...

// Register the ActivitySource as a singleton in the DI container.
// Create a service to expose the ActivitySource Instruments.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// Create a service to expose the ActivitySource Instruments.
// Create a service to expose the ActivitySource.

Unless you plan to add metric instruments as well to Instrumentation.cs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed that line and updated the above comment.

Copy link
Member

@cijothomas cijothomas left a comment

Choose a reason for hiding this comment

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

Thanks. Left some more comments, but okay to address them as follow ups.

@svrnm svrnm requested a review from a team May 16, 2024 06:56
@svrnm svrnm dismissed theletterf’s stale review May 16, 2024 06:59

All changes applied

@svrnm svrnm removed the sig-approval-missing Co-owning SIG didn't provide an approval label May 16, 2024
@svrnm svrnm merged commit 744ef19 into open-telemetry:main May 16, 2024
15 checks passed
@svrnm
Copy link
Member

svrnm commented May 16, 2024

Thanks @IAMebonyhope for all the effort put into this! I merged the PR and this should be live in a minute. I will also create a few follow up issues on the remaining non-blocking feedback

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

Successfully merging this pull request may close these issues.

8 participants