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 SDK and configuration #4966

Merged
merged 22 commits into from
Sep 17, 2024

Conversation

jack-berg
Copy link
Member

@jack-berg jack-berg commented Aug 5, 2024

This is the first PR in what will be a multi-step reworking of the Java docs, as discussed in #4853 and the related doc.

Disclaimer: Refactoring docs iteratively is difficult because its hard to know where to stop. Docs develop organically over time and end with lots of interlinking and little warnings which get in the way of refactoring. For this PR, the scope I targeted was the "SDK", but this leaked into "SDK configuration" because the two are so tightly connected. It also leaked into the "Instrumentation" page which often talks about the SDK through conflated responsibilities. It also leaked into the standalone "Resources", "Exporters", and "Samplers" pages since these are SDK concepts.

What does this PR do?

  • Add a new "Manage Telemetry with SDK" page
    • Standard presentation of all key user facing SDK components. Each component includes a brief description, a table of artifact coordinates of key implementations, a demonstration of programmatic configuration, and a demonstration of custom implementations.
    • This page supersedes "Sampling", "Exporters", and "Resources" pages
    • This page supersedes SDK content sprinkled through the "Instrumentation" page
  • Rework "Configuration" page into "Configure the SDK"
    • Discuss programmatic configuration interface
    • Discuss zero-code SDK autoconfigure module, and recommend to users
      • Enumerate all environment variables / system properties
      • Explain how users can layer on programmatic configuration when properties are insufficient
      • Explain SPI extension points, and for each SPI, provide: a brief description, a table of artifact coordinates of key implementations, and a demonstration of an implementation.
    • Discuss declarative configuration (i.e. file configuration), with caveat that its under development

There is lot of content in this PR, but the principles are simple and scalable. The quantity of content reflects the essential complexity of the project, and is required in order for completeness. I recommend reviewing by looking at the rendered website.

I have future PRs planned for:

  • Add "Learn the basics" page introducing concepts and routing users to correct pages
  • Rework "Instrumentation" page "Use the API", with consistent framing and consistent usage of code snippets.
  • Rework "Libraries" page into "Instrument an application", describing different instrumentations categories and routing users to appropriate places for installation instructions
  • Add "Add Telemetry for Library Developers" page establishing idiomatic practices for library authors looking to adopt OpenTelemetry natively
  • And more...

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

Preview: https://deploy-preview-4966--opentelemetry.netlify.app/docs/languages/java/

@jack-berg
Copy link
Member Author

Corresponding PR with supporting code snippets: open-telemetry/opentelemetry-java-examples#455

Copy link
Member

@zeitlinger zeitlinger left a comment

Choose a reason for hiding this comment

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

great - so much clearer

content/en/docs/languages/java/configuration.md Outdated Show resolved Hide resolved
content/en/docs/languages/java/configuration.md Outdated Show resolved Hide resolved
content/en/docs/languages/java/configuration.md Outdated Show resolved Hide resolved
the SDK. For an exhaustive set of the available configuration APIs, consult the
code.

## Zero-code SDK autoconfigure
Copy link
Member

Choose a reason for hiding this comment

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

the term zero-code already has a meaning: https://opentelemetry.io/docs/concepts/instrumentation/zero-code/

maybe "declarative"?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm trying to rebrand file configuration as "declarative configuration" here: open-telemetry/opentelemetry-specification#4128

Since SDK autoconfigure combines environment variable / system property configuration AND declarative configuration in one module, calling it "declarative configuration" is probably not a great name.

Maybe just "SDK autoconfigure"? Also open to other ideas..

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

Fantastic work! Leaving a first round of suggestions.

content/en/docs/languages/java/configuration.md Outdated Show resolved Hide resolved
content/en/docs/languages/java/configuration.md Outdated Show resolved Hide resolved
content/en/docs/languages/java/configuration.md Outdated Show resolved Hide resolved
content/en/docs/languages/java/configuration.md Outdated Show resolved Hide resolved
content/en/docs/languages/java/configuration.md Outdated Show resolved Hide resolved
content/en/docs/languages/java/configuration.md Outdated Show resolved Hide resolved
content/en/docs/languages/java/configuration.md Outdated Show resolved Hide resolved
content/en/docs/languages/java/configuration.md Outdated Show resolved Hide resolved
content/en/docs/languages/java/configuration.md Outdated Show resolved Hide resolved
content/en/docs/languages/java/configuration.md Outdated Show resolved Hide resolved
@opentelemetrybot opentelemetrybot requested a review from a team August 6, 2024 15:09
@jack-berg
Copy link
Member Author

Thanks for the reviews @theletterf, @zeitlinger!

Copy link
Member

@jaydeluca jaydeluca left a comment

Choose a reason for hiding this comment

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

huge improvement, bravo! 👏

Left some ideas and suggestions for consideration.

content/en/docs/languages/java/sdk.md Outdated Show resolved Hide resolved
content/en/docs/languages/java/sdk.md Outdated Show resolved Hide resolved
content/en/docs/languages/java/sdk.md Outdated Show resolved Hide resolved
content/en/docs/languages/java/sdk.md Outdated Show resolved Hide resolved
content/en/docs/languages/java/sdk.md Outdated Show resolved Hide resolved
content/en/docs/languages/java/sdk.md Outdated Show resolved Hide resolved
content/en/docs/languages/java/sdk.md Outdated Show resolved Hide resolved
content/en/docs/languages/java/sdk.md Outdated Show resolved Hide resolved
content/en/docs/languages/java/configuration.md Outdated Show resolved Hide resolved
content/en/docs/languages/java/configuration.md Outdated Show resolved Hide resolved
@opentelemetrybot opentelemetrybot requested a review from a team August 7, 2024 17:08
@svrnm
Copy link
Member

svrnm commented Aug 8, 2024

Finally, I found some time to take a look, but I see already that @theletterf , @jaydeluca and @zeitlinger are providing reviews and great feedback!

Awesome work, looking forward to see this land.

@chalin chalin force-pushed the refactor-java-configuration branch from fe84b47 to a53743d Compare August 9, 2024 21:29
@jack-berg
Copy link
Member Author

@svrnm any items I need to followup on that I'm missing?

@svrnm
Copy link
Member

svrnm commented Sep 9, 2024

@svrnm any items I need to followup on that I'm missing?

there seems to be a merge conflict, beyond that I would hope for some more reviews from Docs & Java (@theletterf needs to verify the changes requested)

@svrnm
Copy link
Member

svrnm commented Sep 9, 2024

6:21:54 PM: ERROR Param "vers.contrib" not found: "/opt/build/repo/content/en/docs/languages/java/sdk.md:215:80"

Not sure what happened here

@jack-berg
Copy link
Member Author

6:21:54 PM: ERROR Param "vers.contrib" not found: "/opt/build/repo/content/en/docs/languages/java/sdk.md:215:80"

Not sure what happened here

Yeah I don't know either. I can recreate it locally but can't figure out the cause.

@svrnm
Copy link
Member

svrnm commented Sep 11, 2024

let me take a look

@svrnm
Copy link
Member

svrnm commented Sep 11, 2024

this seems to be an issue related to the localizations effort. I can not explain why, but since there is a chinese translation of the Java index page, this is failing. If you update that file the issue goes away:

diff --git a/content/zh/docs/languages/java/_index.md b/content/zh/docs/languages/java/_index.md
index e6c54318..fa112b3e 100644
--- a/content/zh/docs/languages/java/_index.md
+++ b/content/zh/docs/languages/java/_index.md
@@ -9,6 +9,7 @@ cascade:
     instrumentation: 2.7.0
     otel: 1.41.0
     semconv: 1.26.0
+    contrib: 1.38.0

I think as a workaround we can do that, but we might need to revisit how we store those versions

Signed-off-by: svrnm <neumanns@cisco.com>
@opentelemetrybot opentelemetrybot requested a review from a team September 13, 2024 09:23
@svrnm
Copy link
Member

svrnm commented Sep 13, 2024

Made it work again, using the changes as suggested above.

@open-telemetry/java-approvers @open-telemetry/docs-approvers PTAL! Would be great to ship this if there are no more major objections :-)

@opentelemetrybot opentelemetrybot requested review from a team September 17, 2024 14:52
@theletterf theletterf added this pull request to the merge queue Sep 17, 2024
Merged via the queue into open-telemetry:main with commit f1a5dde Sep 17, 2024
17 checks passed
Comment on lines +10 to +12
otel: 1.42.1
contrib: 1.38.0
semconv: 1.27.0
Copy link
Contributor

Choose a reason for hiding this comment

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

@open-telemetry/docs-maintainers - a reminder that an en PR shouldn't be updating non-en pages.

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

Successfully merging this pull request may close these issues.

8 participants