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

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

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

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!

@svrnm
Copy link
Member

svrnm commented Oct 7, 2024

@jack-berg can you check with @open-telemetry/java-approvers that at least one of them is also taking a look through the changes?

content/en/docs/languages/java/api-components.md Outdated Show resolved Hide resolved
content/en/docs/languages/java/api-components.md Outdated Show resolved Hide resolved
content/en/docs/languages/java/api-components.md Outdated Show resolved Hide resolved
content/en/docs/languages/java/api-components.md Outdated Show resolved Hide resolved
content/en/docs/languages/java/api-components.md Outdated Show resolved Hide resolved
content/en/docs/languages/java/api-components.md Outdated Show resolved Hide resolved
content/en/docs/languages/java/api-components.md Outdated Show resolved Hide resolved
content/en/docs/languages/java/api-components.md Outdated Show resolved Hide resolved
content/en/docs/languages/java/api-components.md Outdated Show resolved Hide resolved
content/en/docs/languages/java/api-components.md Outdated Show resolved Hide resolved
@jack-berg
Copy link
Member Author

Thanks for the review @jaydeluca 🙏

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.

overall lgtm, there are a few open discussions we should close and then wrap this up :-)

content-modules/opentelemetry-java-examples Outdated Show resolved Hide resolved
@opentelemetrybot opentelemetrybot requested review from a team October 9, 2024 14:12
@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.

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

Successfully merging this pull request may close these issues.

4 participants