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.

Sign up for GitHub

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

Jump to bottom

Changed getting started docs for .NET to coded instrumentation #3364

Merged
merged 1 commit into from
Nov 8, 2023
Merged

Changed getting started docs for .NET to coded instrumentation #3364

merged 1 commit into from
Nov 8, 2023

Conversation

martinjt
Copy link
Member

@martinjt martinjt commented Oct 11, 2023 •
edited by chalin
Loading

Docs PR Checklist

This reverts the docs to using the .NET coded instrumentation that is more intuitive for .NET developers. This was discussed in the OTel-dotnet SIG and agreed that this is the approach we want to push for as the initial getting started as it's what .NET engineers expect to see.

  • This PR is for a documentation page whose authoritative copy is in the
    opentelemetry.io repository.

Preview: https://deploy-preview-3364--opentelemetry.netlify.app/docs/instrumentation/net/getting-started/

@martinjt martinjt requested review from a team October 11, 2023 11:48
svrnm
svrnm requested changes Oct 11, 2023
View reviewed changes
Copy link
Member

@svrnm svrnm left a comment

Choose a reason for hiding this comment

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

I am wondering if we can take the instructions for automatic instrumentation and move some of it to a page /net/automatic/_index ?

content/en/docs/instrumentation/net/getting-started.md Outdated Show resolved Hide resolved
@cartermp
Copy link
Contributor

I agree, we should move the autoinstrumentation docs to an autoinstrumentation folder. There will be people who would prefer to use it and we should still have that documented, even if it's not the default for getting started.

@chalin chalin force-pushed the dotnet-code-getting-started branch from 092e31e to ba498a8 Compare November 1, 2023 00:00
@svrnm
Copy link
Member

svrnm commented Nov 6, 2023

@martinjt anything you need from us to push this forward? Happy to have this change implemented.

@martinjt
Copy link
Member Author

martinjt commented Nov 6, 2023

Sorry, I'd completely missed that this wasn't finished. Looking now

@martinjt martinjt force-pushed the dotnet-code-getting-started branch 3 times, most recently from 8381a88 to 76bcd4d Compare November 7, 2023 09:53
@martinjt
Copy link
Member Author

martinjt commented Nov 7, 2023

Pretty sure it's ready now.

@martinjt
Copy link
Member Author

martinjt commented Nov 7, 2023

@svrnm do let me know if you need anything else, might be easier to ping me on slack though

svrnm
svrnm requested changes Nov 7, 2023
View reviewed changes
Copy link
Member

@svrnm svrnm left a comment

Choose a reason for hiding this comment

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

A few minor changes, but overall lgtm.

The piece missing for me is that the instructions for the automatic instrumentation are still something worth keeping, so maybe as part of https://opentelemetry.io/docs/instrumentation/net/automatic/ there should be a section auto instrumenting the Getting Started app. Would this be something you can add to this PR or should we move this out into a separate one?

@cartermp
Copy link
Contributor

cartermp commented Nov 7, 2023

@svrnm let's keep this one smaller and make additional notes w.r.t. automatic instrumentation in a different PR.

@svrnm svrnm requested review from chalin and utpilla November 8, 2023 10:02
@svrnm svrnm requested a review from cijothomas November 8, 2023 10:07
svrnm
svrnm approved these changes Nov 8, 2023
View reviewed changes
Copy link
Member

@svrnm svrnm left a comment

Choose a reason for hiding this comment

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

Docs-wise this looks good, @open-telemetry/dotnet-approvers PTAL!

@pellared
Copy link
Member

pellared commented Nov 8, 2023

Assuming that this is merged: who (and when) wants to add getting started docs for automatic instrumentation?

@svrnm
Copy link
Member

svrnm commented Nov 8, 2023

Assuming that this is merged: who (and when) wants to add getting started docs for automatic instrumentation?

If nobody is going to take it, I will raise an issue and try to find some time to make a copy of what we have for automatic today

@martinjt martinjt force-pushed the dotnet-code-getting-started branch from 25176c0 to f45b810 Compare November 8, 2023 13:32
chalin
chalin approved these changes Nov 8, 2023
View reviewed changes
Copy link
Contributor

@chalin chalin left a comment

Choose a reason for hiding this comment

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

LGTM

@svrnm svrnm merged commit 2a68528 into open-telemetry:main Nov 8, 2023
12 checks passed
@svrnm
Copy link
Member

svrnm commented Nov 8, 2023

🎉

@svrnm svrnm mentioned this pull request Nov 8, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pellared pellared pellared approved these changes

@chalin chalin chalin approved these changes

@svrnm svrnm svrnm approved these changes

@utpilla utpilla Awaiting requested review from utpilla

@cijothomas cijothomas Awaiting requested review from cijothomas

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@martinjt @cartermp @svrnm @pellared @chalin @cijothomas @utpilla