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

Move the Spring Auto-Configuration doc #3900

Merged

Conversation

jaydeluca
Copy link
Member

@jaydeluca jaydeluca commented Jan 31, 2024

(Continuation from #3758 after making a mistake rebasing)

Related to open-telemetry/opentelemetry-java-instrumentation#10083

Move and adapt the documentation Readme: main/instrumentation/spring/spring-boot-autoconfigure/README.md


after rebasing I am now getting the following textlint errors:

  159:23  ✓ error  Incorrect usage of the term: “auto configuration”, use “autoconfiguration” instead    terminology
  172:6   ✓ error  Incorrect usage of the term: “auto-configuration”, use “autoconfiguration” instead    terminology
  238:1   ✓ error  Incorrect usage of the term: “Auto-configures”, use “Autoconfigures” instead          terminology
  251:17  ✓ error  Incorrect usage of the term: “Auto Configuration”, use “autoConfiguration” instead    terminology
  262:21  ✓ error  Incorrect usage of the term: “Auto Configuration”, use “autoConfiguration” instead    terminology
  273:21  ✓ error  Incorrect usage of the term: “Auto Configuration”, use “autoConfiguration” instead    terminology
  275:10  ✓ error  Incorrect usage of the term: “auto-configurations”, use “autoconfigurations” instead  terminology
  278:6   ✓ error  Incorrect usage of the term: “auto-configuration”, use “autoconfiguration” instead    terminology
  486:1   ✓ error  Incorrect usage of the term: “Auto-configuration”, use “Autoconfiguration” instead    terminology
  507:37  ✓ error  Incorrect usage of the term: “auto-configuration”, use “autoconfiguration” instead    terminology

"Auto-configuration" is how it is referred to in the spring boot docs so I am hesitant to just go and update everything to "autoconfiguration" without checking first. @jeanbisutti Do you know how I should proceed?


Preview: https://deploy-preview-3900--opentelemetry.netlify.app/docs/languages/java/automatic/spring-boot/

@trask
Copy link
Member

trask commented Jan 31, 2024

I think ok to go ahead and change them. We have similar linting in Microsoft docs that always gets me with auto-instrumentation --> autoinstrumentation.

@jeanbisutti
Copy link
Member

@jaydeluca I see #3903 that seems to fix the spelling issues.

@chalin
Copy link
Contributor

chalin commented Jan 31, 2024

@jaydeluca I see #3903 that seems to fix the spelling issues.

Actually, after #3903 lands, you'll be able to fix the spelling issue automatically using this command:

$ npm run check:text -- --fix

(You can run that command already, but you'll need to tweak a few instances of the rewritten words.)

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.

We're getting closer! See inline for copyedits.

content/en/docs/languages/java/automatic/spring-boot.md Outdated Show resolved Hide resolved
content/en/docs/languages/java/automatic/spring-boot.md Outdated Show resolved Hide resolved
content/en/docs/languages/java/automatic/spring-boot.md Outdated Show resolved Hide resolved
content/en/docs/languages/java/automatic/spring-boot.md Outdated Show resolved Hide resolved
content/en/docs/languages/java/automatic/spring-boot.md Outdated Show resolved Hide resolved
content/en/docs/languages/java/automatic/spring-boot.md Outdated Show resolved Hide resolved
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.

Regarding the organisation / structure of this page. If you look at the TOC currently, it looks like this:

image

That doesn't make sense to me to have a single top-level heading with pretty much all of the page content under that single heading. As mentioned at the start of this page "this page documents the OpenTelemetry starter" so I think that we should:

  • Change the page title to "Spring Boot Starter", to match the actual content of the page
  • The heading "OpenTelemetry Spring starter" should be dropped
  • All other headings should be promoted one level up.

@jeanbisutti
Copy link
Member

Regarding the organisation / structure of this page. If you look at the TOC currently, it looks like this:

image That doesn't make sense to me to have a single top-level heading with pretty much all of the page content under that single heading. As mentioned at the start of this page "this page documents the OpenTelemetry starter" so I think that we should:
  • Change the page title to "Spring Boot Starter", to match the actual content of the page
  • The heading "OpenTelemetry Spring starter" should be dropped
  • All other headings should be promoted one level up.

@chalin There is a mistake. Thanks for having pointed this. Other configurations should be a top level heading.

It may have another topic. The Spring Boot page should probably only contain this section explaining when to use the Java agent or the starter:
image

The rest of the page could propably move to another one related to starters and autoconfigurations. Perhaps it could be done in another PR? I could take care of it.

@chalin
Copy link
Contributor

chalin commented Jan 31, 2024

The rest of the page could propably move to another one related to starters and autoconfigurations. Perhaps it could be done in another PR? I could take care of it.

SGTM, and I'm ok with approaching this incrementally. Please create an issue to track the required changes: you can simply create a new issue from #3900 (comment).

I'll wait for my other comments to be addressed before making a (final) review pass. Thanks!

@jeanbisutti
Copy link
Member

Please create an issue to track the required changes: you can simply create a new issue from #3900 (comment).

Done: #3911

…e code references, fix links, other comments
@jaydeluca
Copy link
Member Author

I really appreciate all the help from ya'll 🙏

@jaydeluca jaydeluca marked this pull request as ready for review January 31, 2024 23:05
@jaydeluca jaydeluca requested review from a team January 31, 2024 23:05
@jeanbisutti jeanbisutti requested a review from chalin February 1, 2024 10:10
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.

❤️

So, @jeanbisutti, this actually closes #3911?

@chalin chalin added the sig-approval-missing Co-owning SIG didn't provide an approval label Feb 1, 2024
@chalin
Copy link
Contributor

chalin commented Feb 1, 2024

@open-telemetry/java-approvers PTAL

@jeanbisutti
Copy link
Member

❤️

So, @jeanbisutti, this actually closes #3911?

@chalin It does not close #3911. All the non-agent doc is still in the Spring Boot page.

@jeanbisutti
Copy link
Member

@open-telemetry/docs-approvers FYI, the Spring Boot page documents instrumentation features for Spring Boot and developed in the https://github.com/open-telemetry/opentelemetry-java-instrumentation repository.

@trask
Copy link
Member

trask commented Feb 1, 2024

@open-telemetry/java-approvers PTAL

fyi @jeanbisutti is in @open-telemetry/java-instrumentation-approvers now

@cartermp cartermp removed the sig-approval-missing Co-owning SIG didn't provide an approval label Feb 1, 2024
@cartermp
Copy link
Contributor

cartermp commented Feb 1, 2024

Aaaaand we're in!

@cartermp cartermp merged commit 12b42d4 into open-telemetry:main Feb 1, 2024
14 checks passed
@trask
Copy link
Member

trask commented Feb 1, 2024

thx @jaydeluca ❤️

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.

5 participants