-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Improve Java OpenTelemetry docs #13202
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
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
2 Skipped Deployments
|
Bundle ReportChanges will increase total bundle size by 2.79kB (0.01%) ⬆️. This is within the configured threshold ✅ Detailed changes
Affected Assets, Files, and Routes:view changes for bundle: sentry-docs-server-cjsAssets Changed:
view changes for bundle: sentry-docs-client-array-pushAssets Changed:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the new more digestible structure and also the fact that this is gonna be much easier to find in the docs.
Overall it feels that we're assuming that the person who's reading this is already using our SDK and not using OTEL.
I would briefly mention the benefits you get on the other way around (should be obvious but you will benefit from your spans being sent to Sentry and all our features plus error monitoring) plus how it works (just need to mention that any spans you create using OTEL APIs are automatically sent to Sentry just by installing the SDK -- that's how it works in that case right?).
Maybe there's a better way to explain this using OTEL terms that they would already be familiar with, like "adding the Sentry SDK as a dependency will autoconfigure OTEL to export telemetry data to Sentry and the SDK will act as the exporter" or something like that (not sure if that's accurate though).
This is because I assume a lot of people reading this page will likely already have an OTEL setup going (as opposed to a Sentry setup going and willing to use OTEL).
Also there's some redundancy between the pages. If I navigate to /platforms/java/opentelemetry/setup/ I see the part about the agent (/platforms/java/opentelemetry/setup/agent/) which maybe you didn't intend to have show up in both places.
I've created getsentry/sentry-java#4313 so we can further improve docs and explain what customers gain when adding Sentry.
I did that on purpose to explain the agent in both locations. On the /setup page it's supposed to help figure out whether you want to use the Agent and on /setup/agent I didn't want to skip it because we're likely going to link to this page to tell customers what to use so I didn't want that page to leave out the explanation either. |
lcian
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approving to unblock, we can improve this over time
lbloder
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good overall.
Much more digestible this way 👍
Left a few comments on some minor things
platform-includes/performance/opentelemetry-setup/agent-explanation/java.mdx
Outdated
Show resolved
Hide resolved
|
|
||
| ## Install | ||
|
|
||
| <PlatformContent includePath="performance/opentelemetry-install/without-java-agent" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this include, we show Information about how to install without agent, and have a paragraph that mentions that there is a separate dependency for spring boot. The link to which points to: /platforms/java/guides/spring-boot/tracing/instrumentation/opentelemetry/. But, in my opinion, should point to: /platforms/java/guides/spring-boot/opentelemetry/setup/agentless/. WDYT?
|
|
||
| ### Capturing HTTP Headers | ||
|
|
||
| By default OpenTelemetry does not capture any HTTP headers. This, however, can be configured using system properties or environment variables as per OpenTelemetry's configuration documentation [here](https://opentelemetry.io/docs/zero-code/java/agent/instrumentation/http/#capturing-http-request-and-response-headers). Each variable is a comma-separated list of HTTP header names that should be captured. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like something I forgot initially when creating this section. But should we add a disclaimer here to look out for PII in the headers?
| sidebar_order: 200 | ||
| --- | ||
|
|
||
| If you do not want to use our recommended <PlatformLink to="/opentelemetry/setup/agent">Java Agent</PlatformLink>, we also offer a dependency that allows you to use OpenTelemetry with Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we add an example use case here for when to use the agentless version instead of the agent, like when using GraalVM?
Could also be done in getsentry/sentry-java#4313
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah let's get this merged and improve later.
Co-authored-by: Lukas Bloder <[email protected]>
lbloder
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
* Improve Java OpenTelemetry docs * try to fix lint * try to fix lint2 * Apply suggestions from code review Co-authored-by: Lukas Bloder <[email protected]> * PR feedback --------- Co-authored-by: Lukas Bloder <[email protected]>
Closes getsentry/sentry-java#4104 and #12619
DESCRIBE YOUR PR
Tell us what you're changing and why. If your PR resolves an issue, please link it so it closes automatically.
IS YOUR CHANGE URGENT?
Help us prioritize incoming PRs by letting us know when the change needs to go live.
SLA
Thanks in advance for your help!
PRE-MERGE CHECKLIST
Make sure you've checked the following before merging your changes:
LEGAL BOILERPLATE
Look, I get it. The entity doing business as "Sentry" was incorporated in the State of Delaware in 2015 as Functional Software, Inc. and is gonna need some rights from me in order to utilize my contributions in this here PR. So here's the deal: I retain all rights, title and interest in and to my contributions, and by keeping this boilerplate intact I confirm that Sentry can use, modify, copy, and redistribute my contributions, under Sentry's choice of terms.
EXTRA RESOURCES