Cassandra instrumentation #1712

felixbarny · 2021-03-22T07:36:42Z

What does this PR do?

Checklist

I have updated CHANGELOG.asciidoc
My code follows the style guidelines of this project
I have made corresponding changes to the documentation
I have added tests that prove my fix is effective or that my feature works
New and existing unit tests pass locally with my changes
I have updated supported-technologies.asciidoc
~~[ ] Added an API method or config option? Document in which version this will be introduced~~
Added an instrumentation plugin? Describe how you made sure that old, non-supported versions are not instrumented by accident.
The type matcher excludes 1.x drivers

ghost · 2021-03-22T07:41:05Z

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS

Expand to view the summary

Build stats

Build Cause: Started by user eyalkoren
Start Time: 2021-04-01T08:43:38.383+0000
Duration: 55 min 30 sec
Commit: 82426f3

Test stats 🧪

Test	Results
Failed	0
Passed	1982
Skipped	19
Total	2001

Trends 🧪

💚 Flaky test report

Tests succeeded.

Expand to view the summary

Test stats 🧪

Test	Results
Failed	0
Passed	1982
Skipped	19
Total	2001

eyalkoren

Awesome capability addition! ❤️

apm-agent-core/src/main/java/co/elastic/apm/agent/db/signature/SignatureParser.java

eyalkoren · 2021-03-30T14:33:42Z

apm-agent-core/src/main/java/co/elastic/apm/agent/impl/context/Destination.java

+ }
+ }
+
+ public void withInetAddress(InetAddress inetAddress) {


Not using getHostAddress() to prevent allocations? This implementation is IPv4-specific. But even better would be getHostName(), wouldn't it?
I see the value of being efficient here, as it will be called for each span, but I think the better option is to cache addresses per com.datastax.driver.core.Host instead in the Cassandra instrumentations.

Ah, right, I didn't consider IPv6.
getHostName will make a DNS lookup, I'm not sure we'd want to do this as it could introduce quite some overhead.

getHostName will make a DNS lookup, I'm not sure we'd want to do this as it could introduce quite some overhead

That's right, but it is preferable than IP, so I suggested to cache the result based on the Host

We should probably do the same in the RabbitMQ instrumentation -

apm-agent-java/apm-agent-plugins/apm-rabbitmq/apm-rabbitmq-plugin/src/main/java/co/elastic/apm/agent/rabbitmq/ChannelInstrumentation.java

Line 299 in 789406b

destination.withAddress(connection.getAddress().getHostName());

(caching the address per com.rabbitmq.client.Connection).
Whether you implement this caching or not, please switch the RabbitMQ instrumentation to use your new co.elastic.apm.agent.impl.context.Destination#withInetAddress utility.

unified address handling, not only in RabbitMQ

I think it's the only other one relying on InetAddress

apm-agent-core/src/test/java/co/elastic/apm/agent/MockReporter.java

...ssandra3-plugin/src/main/java/co/elastic/apm/agent/cassandra3/Cassandra3Instrumentation.java

eyalkoren · 2021-03-31T10:42:51Z

...ssandra3-plugin/src/main/java/co/elastic/apm/agent/cassandra3/Cassandra3Instrumentation.java

+ }
+ final Span span = (Span) spanObj;
+ span.captureException(thrown).deactivate();
+ Futures.addCallback(result, new FutureCallback<ResultSet>() {


Cool!
It relies on a non-explicit dependency that is marked with @com.google.common.annotations.Beta annotation, but not sure this means actual instability. Also, in addition to this FutureCallback allocation, there is an additional Runnable object allocation internally.
In Apache HTTP we wrap futures and pool these wrappers to maintain zero allocation.

Since this is so slick and simple, it may worth both "risks". I just wanted to point out and let you decide.

I've tested with a bunch of versions and this was quite stable. However, there's no stable API to get the direct executor, so we'd have to implement that in the Cassandra module if we wanted to directly implement the Runnable. I tried to keep the complexity of this module low.

Yes, this simplicity is very appealing. I agree that implementing it is a worse option.
My suggested alternative was wrapping the Future with pooled futures. If it is simple enough, we can do that (and save two allocations per query), otherwise this is good enough.

eyalkoren · 2021-03-31T10:49:25Z

...ssandra4-plugin/src/main/java/co/elastic/apm/agent/cassandra4/Cassandra4Instrumentation.java

+ private static void setDestination(Span span, ExecutionInfo info) {
+ Node coordinator = info.getCoordinator();
+ if (coordinator != null) {
+ span.getContext().getDestination().withSocketAddress(coordinator.getEndPoint().resolve());


Same as above - if makes sense, we can cache destination based on Node or EndPoint.

If we cache, we should probably do it based on the socket address in Destination so that it's not Cassandra-specific. I left it out for now.

What I had in mind is provide the API with an Object to cache for, so it is not Cassandra-specific

apm-agent-plugins/apm-cassandra/apm-cassandra4-plugin/pom.xml

...sandra4-plugin/src/test/java/co/elastic/apm/agent/cassandra/Cassandra4InstrumentationIT.java

...sandra3-plugin/src/test/java/co/elastic/apm/agent/cassandra/Cassandra3InstrumentationIT.java

apm-agent-core/src/main/java/co/elastic/apm/agent/db/signature/SignatureParser.java

removing destination checks, assuming that the generic validation that the fields are non-empty is good enough

codecov-io · 2021-03-31T17:21:17Z

Codecov Report

Merging #1712 (6cb9019) into master (1748259) will increase coverage by 0.56%.
The diff coverage is 40.62%.

@@ Coverage Diff @@ ## master #1712 +/- ## ============================================ + Coverage 61.30% 61.87% +0.56%  + Complexity 3621 3469 -152  ============================================ Files 401 381 -20 Lines 18250 17417 -833 Branches 2507 2406 -101 ============================================ - Hits 11188 10776 -412  + Misses 6278 5887 -391  + Partials 784 754 -30

Impacted Files	Coverage Δ	Complexity Δ
...va/co/elastic/apm/agent/impl/ElasticApmTracer.java	`65.85% <0.00%> (-0.82%)`	`83.00 <0.00> (ø)`
...n/java/co/elastic/apm/agent/impl/GlobalTracer.java	`41.46% <0.00%> (-1.04%)`	`11.00 <0.00> (ø)`
...ain/java/co/elastic/apm/agent/impl/NoopTracer.java	`5.26% <0.00%> (-0.30%)`	`1.00 <0.00> (ø)`
...rc/main/java/co/elastic/apm/agent/impl/Tracer.java	`100.00% <ø> (ø)`	`0.00 <0.00> (ø)`
...co/elastic/apm/agent/impl/context/Destination.java	`75.34% <0.00%> (-13.37%)`	`21.00 <0.00> (ø)`
...astic/apm/agent/impl/transaction/AbstractSpan.java	`75.26% <0.00%> (-0.82%)`	`63.00 <0.00> (ø)`
...stic/apm/agent/jdbc/ConnectionInstrumentation.java	`72.72% <0.00%> (ø)`	`4.00 <0.00> (ø)`
...co/elastic/apm/agent/jdbc/JdbcInstrumentation.java	`100.00% <ø> (+72.72%)`	`3.00 <0.00> (ø)`
...astic/apm/agent/jdbc/StatementInstrumentation.java	`36.63% <0.00%> (ø)`	`4.00 <0.00> (ø)`
...a/co/elastic/apm/agent/jdbc/helper/JdbcHelper.java	`0.00% <0.00%> (ø)`	`0.00 <0.00> (ø)`
... and 34 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1748259...6cb9019. Read the comment docs.

eyalkoren · 2021-04-01T09:36:35Z

...ssandra3-plugin/src/main/java/co/elastic/apm/agent/cassandra3/Cassandra3Instrumentation.java

+ try {
+ Class.forName("com.datastax.driver.core.Host").getMethod("getSocketAddress");
+ delegate = localDelegate = (DestinationAddressSetter) Class.forName(DestinationAddressSetter.class.getName() + "$WithSocketAddress").getEnumConstants()[0];
+ } catch (ReflectiveOperationException | LinkageError ignore) {
+ delegate = localDelegate = WithInetAddress.INSTANCE;
+ }


That looks good!
Do you have some test app where this can be tested with class loading that better resembles real scenarios?

I have manually tested with a standalone Spring Boot application:

Click to expand

@SpringBootApplication public class CassandraDemoApplication { public static void main(String[] args) { SpringApplication.run(CassandraDemoApplication.class, args); } @Bean public Cluster cluster(@Value("${cassandra.port}") int cassandraPort) { Cluster cluster = Cluster.builder() .addContactPoint("localhost") .withPort(cassandraPort) .withoutMetrics() .build(); try (Session s = cluster.connect()) { s.execute("CREATE KEYSPACE IF NOT EXISTS test WITH replication = {'class':'SimpleStrategy','replication_factor':'1'};"); } try (Session s = cluster.connect("test")) { s.execute("CREATE TABLE users (id UUID, name text, PRIMARY KEY(name, id))"); s.execute(s.prepare("INSERT INTO users (id, name) values (?, ?)").bind(UUID.randomUUID(), "alice")); } catch (Exception ignore) { } return cluster; } @RestController public static class HomeController { private final Cluster cluster; public HomeController(@Autowired Cluster cluster) { this.cluster = cluster; } @GetMapping("/") public String home() { return "ok"; } @GetMapping("/cassy") public String cassandra() { try (Session s = cluster.connect("test")) { return s.execute("SELECT * FROM users where name = 'alice' ALLOW FILTERING").one().getString("name"); } } } }

With version 2.0.1, it only records destination.address, and version 2.0.2 also records destination.port.

…va into feature/java-stable-tag * 'feature/java-stable-tag' of github.com:v1v/apm-agent-java: (534 commits) Added Support for JBoss Logging to the log correlation plugin (elastic#1737) Removing improper RabbitMQ reference from instrumentation class (elastic#1745) Loading Advice classes lazily when required (elastic#1736) add support for ibm JVMs (elastic#1739) Adds circuit breaker to app deployment in load pipeline (elastic#1731) Bump json-schema-validator from 1.0.50 to 1.0.51 (elastic#1732) Bump java-driver-core from 4.10.0 to 4.11.0 (elastic#1733) Cassandra instrumentation (elastic#1712) Build apm-agent-attach-cli before application-server-integration-tests (elastic#1729) Fix thread pool method matchers (elastic#1717) Always use English locale when formatting doubles (elastic#1727) (elastic#1728) feat: grab locust metrics on load tests (elastic#1724) log readonly msg errors only in debug (elastic#1715) feat: use metricbeat to grab host metrics (elastic#1718) Eliminate log4j shaders dependency in slf4j (elastic#1723) Fix log correlation for log4j2 (elastic#1720) Allowing more time for the test Bump json-schema-validator from 1.0.49 to 1.0.50 (elastic#1711) Fix master build and print URL on connection error Attacher improvements (elastic#1667) ...

Cassandra instrumentation

ee2a36e

github-actions bot added the agent-java label Mar 22, 2021

felixbarny added 2 commits March 22, 2021 08:37

Add changelog

3b16a20

Merge remote-tracking branch 'origin/master' into cassandra

c280e42

felixbarny and others added 5 commits March 22, 2021 11:51

Merge remote-tracking branch 'origin/master' into cassandra

f08a469

Fix animal sniffer error

4e17bc0

Update configuration.asciidoc

d9639f5

Merge branch 'master' into cassandra

3143157

Set scope to provided

695baa2

AlexanderWert added this to the 7.13 milestone Mar 29, 2021

felixbarny and others added 4 commits March 29, 2021 12:07

Merge remote-tracking branch 'origin/master' into cassandra

8d32a87

Merge remote-tracking branch 'upstream/master' into cassandra

42e06a8

Merge remote-tracking branch 'upstream/master' into cassandra

6801323

Fix versions

c7c9eda

eyalkoren reviewed Mar 31, 2021

View reviewed changes

felixbarny added 6 commits March 31, 2021 14:48

Implement review suggestions

afd8524

Unify destination address handling

b411c30

Fix RabbitMQ tests

1d0df45

Make javac happy

5cebf85

Another attempt to make RabbitMQ tests pass

e26be0a

removing destination checks, assuming that the generic validation that the fields are non-empty is good enough

Merge remote-tracking branch 'origin/master' into cassandra

6cb9019

Fix RabbitMQ and Spring AMQP tests

82426f3

eyalkoren approved these changes Apr 1, 2021

View reviewed changes

felixbarny merged commit 60ca9ca into elastic:master Apr 1, 2021

felixbarny deleted the cassandra branch April 1, 2021 13:46

felixbarny mentioned this pull request Apr 15, 2021

test: synchronizing json spec #1761

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Cassandra instrumentation #1712

Cassandra instrumentation #1712

Uh oh!

felixbarny commented Mar 22, 2021 •

edited by zube bot

Loading

ghost commented Mar 22, 2021 •

edited by ghost

Loading

Build stats

Test stats 🧪

Trends 🧪

Test stats 🧪

eyalkoren left a comment

Uh oh!

eyalkoren Mar 30, 2021

felixbarny Mar 31, 2021

eyalkoren Mar 31, 2021

eyalkoren Mar 31, 2021 •

edited

Loading

felixbarny Mar 31, 2021

eyalkoren Mar 31, 2021

Uh oh!

Uh oh!

eyalkoren Mar 31, 2021

felixbarny Mar 31, 2021

eyalkoren Mar 31, 2021

eyalkoren Mar 31, 2021

felixbarny Mar 31, 2021

eyalkoren Mar 31, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codecov-io commented Mar 31, 2021

eyalkoren Apr 1, 2021

felixbarny Apr 1, 2021

Labels

4 participants

Cassandra instrumentation #1712

Cassandra instrumentation #1712

Uh oh!

Conversation

felixbarny commented Mar 22, 2021 • edited by zube bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this PR do?

Checklist

ghost commented Mar 22, 2021 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

💚 Build Succeeded

Build stats

Test stats 🧪

Trends 🧪

💚 Flaky test report

Test stats 🧪

eyalkoren left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eyalkoren Mar 31, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codecov-io commented Mar 31, 2021

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Labels

4 participants

felixbarny commented Mar 22, 2021 •

edited by zube bot

Loading

ghost commented Mar 22, 2021 •

edited by ghost

Loading

eyalkoren Mar 31, 2021 •

edited

Loading