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

Refactor java instrumentation #5276

Merged
merged 13 commits into from
Oct 17, 2024
Merged

Conversation

jack-berg
Copy link
Member

@jack-berg jack-berg commented Sep 30, 2024

Related to #5211. Followup to #4966.

This PR breaks up and adds coherence to the topics currently discussed in Instrumentation:

  • Add a new "Record Telemetry with API" page
    • Standard presentation of all key user facing API components. Each component includes a brief description, links to javadoc, links to relevant concept resoruces, and a code snippet exploring API usage.
    • This page supersedes and expands on the "Traces" and "Metrics" sections from the "Instrumentation" page.
  • Rework "Instrumentation" page into "Instrumentation ecosystem"
    • Discuss the various categories of instrumentation in the OpenTelemetry Java ecosystem, linking out to relevant resources.
    • Discuss cross-cutting instrumentation topics, including context propagation, semantic conventions, and the special case of logging instrumentation.

The PR for the code snippet additions to opentelemetry-java-examples is available here: open-telemetry/opentelemetry-java-examples#498

There is a lot of content in this PR, but like #4966, the principles are simple and scalable. I recommend reviewing by looking at the rendered website. Links to previews for main pages involved:

cc @open-telemetry/java-approvers, @open-telemetry/java-instrumentation-approvers


Redirect test:

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.

@jack-berg - please rebase and resolve conflicts before we proceed:

image

@jack-berg
Copy link
Member Author

@jack-berg - please rebase and resolve conflicts before we proceed:

I've merged main rather than rebase. I generally avoid rebasing since it overwrites git history and requires force pushing. If this repo prefers rebasing for some reason, I'm happy to do that.

@opentelemetrybot opentelemetrybot requested review from a team October 1, 2024 15:10
@opentelemetrybot opentelemetrybot requested review from codeboten and removed request for a team October 1, 2024 15:10
@jack-berg
Copy link
Member Author

@chalin / @svrnm any idea what's happening with a the deploy to netlify? It almost seems like its in a corrupt state, since this PR currently doesn't touch the submodules yet those seem to be the cause of the issue:

Screenshot 2024-10-01 at 10 16 40 AM

@chalin
Copy link
Contributor

chalin commented Oct 1, 2024

@jack-berg - please rebase and resolve conflicts before we proceed:

I've merged main rather than rebase. I generally avoid rebasing since it overwrites git history and requires force pushing. If this repo prefers rebasing for some reason, I'm happy to do that.

Yes rebase from a freshly updated upstream main. Otherwise, we can get wonky issues arise when the PR rooted too far behind, for example: #5270 (review)

@chalin
Copy link
Contributor

chalin commented Oct 1, 2024

@chalin / @svrnm any idea what's happening with a the deploy to netlify? It almost seems like its in a corrupt state, since this PR currently doesn't touch the submodules yet those seem to be the cause of the issue: ...

This is the kind of wonkiness that can happen. Please rebase from a freshly updated upstream main and we'll reassess after.

@jack-berg
Copy link
Member Author

This is the kind of wonkiness that can happen. Please rebase from a freshly updated upstream main and we'll reassess after.

That did the trick. Thanks!

@jack-berg
Copy link
Member Author

We have one remaining open conversation here. Discussing here because that thread is buried and hard to find.

I feel like the easiest path forward is to just remove the links from the translated versions of libraries to the now removed java sections, like I had originally done. Probably not a solution to the general case, but in this case, there are no translated versions of the instrumentation page being modified, and the link probably shouldn't have been there in the first place.

WDYT?

@svrnm
Copy link
Member

svrnm commented Oct 16, 2024

We have one remaining open conversation here. Discussing here because that thread is buried and hard to find.

I feel like the easiest path forward is to just remove the links from the translated versions of libraries to the now removed java sections, like I had originally done. Probably not a solution to the general case, but in this case, there are no translated versions of the instrumentation page being modified, and the link probably shouldn't have been there in the first place.

WDYT?

We need a quick solution to merge this PR, and we need a long term solution to ensure that we can run translations while updating the (English) documentation. For the quick solution, I'd like to lean on whatever @chalin suggests to do, to get this PR merged.

@chalin
Copy link
Contributor

chalin commented Oct 17, 2024

We need a quick solution to merge this PR, and we need a long term solution to ensure that we can run translations while updating the (English) documentation. For the quick solution, I'd like to lean on whatever @chalin suggests to do, to get this PR merged.

From the GH checks:

blog/2023/spring-native/index.html
  target does not exist --- blog/2023/spring-native/index.html --> /docs/languages/java/libraries/

This needs to be fixed, FYI. Because all en pages must use canonical paths.
I'll update it.

As for non-en pages, a general strategy (I'm proposing) is it create a temporary link-checker ignore rule for the problematic pages, along with an issue to track its removal. I'll do that too so illustrate how that look for this PR.

@opentelemetrybot opentelemetrybot requested a review from a team October 17, 2024 00:12
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.

All GH checks are passing now. I don't have time to further review the PR, so I'll lean on @svrnm et all for the rest. Thanks for pushing this forward @jack-berg! I'll mark as approved for the checking & layouts aspects.

@jack-berg
Copy link
Member Author

Thanks for all your help @chalin!

@svrnm
Copy link
Member

svrnm commented Oct 17, 2024

I love it! ❤️

@svrnm svrnm added this pull request to the merge queue Oct 17, 2024
Merged via the queue into open-telemetry:main with commit 2394fa1 Oct 17, 2024
17 checks passed
@trask
Copy link
Member

trask commented Oct 17, 2024

🎉

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

Successfully merging this pull request may close these issues.

5 participants