-
Notifications
You must be signed in to change notification settings - Fork 1k
Add shouldStart checks to lettuce-4.0 and lettuce-5.0 javaagent instrumentation to prevent span creation when disabled #14666
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
|
@trask 👋 This repository doesn't have Copilot instructions. With Copilot instructions, I can understand the repository better, work faster and produce higher quality PRs. I can generate a .github/copilot-instructions.md file for you automatically. Click here to open a pre-filled issue and assign it to me. I'll write the instructions, and then tag you for review. |
Co-authored-by: trask <[email protected]>
…rt implementation Co-authored-by: trask <[email protected]>
|
worth a try 🤷♂️ |
|
@copilot this issue is about lettuce-4.0, not lettuce-5.1 |
You're absolutely right - the issue is about lettuce-4.0, not lettuce-5.1. I was working on the wrong version. I'm now focusing on fixing the shouldStart pattern in lettuce-4.0 (and lettuce-5.0 which has the same issue) javaagent instrumentation files. The lettuce-5.1 version uses library instrumentation which already follows the correct pattern. Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
…rumentations Co-authored-by: trask <[email protected]>
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.
@copilot revert the changes to lettuce-5.1
Co-authored-by: trask <[email protected]>
Done! I've reverted all lettuce-5.1 changes as requested (commit d95c3ac). The fix now focuses solely on lettuce-4.0 and lettuce-5.0 javaagent instrumentations, which is what the original issue #14665 was about. Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
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, just a few minor suggestions to add @Nullable where relevant in the modified code.
...va/io/opentelemetry/javaagent/instrumentation/lettuce/v5_0/LettuceClientInstrumentation.java
Outdated
Show resolved
Hide resolved
...pentelemetry/javaagent/instrumentation/lettuce/v5_0/LettuceAsyncCommandsInstrumentation.java
Outdated
Show resolved
Hide resolved
...a/io/opentelemetry/javaagent/instrumentation/lettuce/v4_0/LettuceConnectInstrumentation.java
Outdated
Show resolved
Hide resolved
...pentelemetry/javaagent/instrumentation/lettuce/v4_0/LettuceAsyncCommandsInstrumentation.java
Outdated
Show resolved
Hide resolved
Co-authored-by: SylvainJuge <[email protected]>
|
🔧 The result from spotlessApply was committed to the PR branch. |
…umentation to prevent span creation when disabled (open-telemetry#14666) Co-authored-by: copilot-swe-agent[bot] <[email protected]> Co-authored-by: trask <[email protected]> Co-authored-by: Trask Stalnaker <[email protected]> Co-authored-by: SylvainJuge <[email protected]> Co-authored-by: otelbot <[email protected]>
The lettuce javaagent instrumentations (versions 4.0 and 5.0) were creating spans even when instrumentation was disabled, bypassing the standard
shouldStartpattern used by other OpenTelemetry instrumentations. This resulted in unnecessary span creation overhead and inconsistent behavior.Problem
The lettuce javaagent instrumentations (versions 4.0 and 5.0) were directly creating spans without checking if instrumentation should be enabled:
Javaagent Instrumentation (lettuce-4.0, lettuce-5.0):
This meant spans would be created even when users wanted to disable lettuce instrumentation, leading to:
Solution
Added proper
shouldStartchecks using the standardInstrumenterpattern consistent with other OpenTelemetry instrumentations:Javaagent Instrumentation (lettuce-4.0, lettuce-5.0):
Key Changes
Javaagent Instrumentation (lettuce-4.0, lettuce-5.0):
LettuceAsyncCommandsInstrumentationand connect instrumentation classes to checkshouldStartbefore creating spansUsage
Javaagent Usage (lettuce-4.0, lettuce-5.0):
The javaagent instrumentations now respect the standard OpenTelemetry instrumentation enablement configuration and will not create spans when instrumentation is disabled.
The implementation follows the same pattern used in other instrumentations like Ratpack, JSP, and Geode, ensuring consistency across the OpenTelemetry Java instrumentation ecosystem.
Fixes #14665.
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.