Added (initial) support for compressing spans #2477

tobiasstadler · 2022-02-18T17:18:11Z

What does this PR do?

Checklist

This is an enhancement of existing features, or a new feature in existing plugins
- I have updated CHANGELOG.asciidoc
- I have added tests that prove my fix is effective or that my feature works
- Added an API method or config option? Document in which version this will be introduced
- I have made corresponding changes to the documentation

github-actions · 2022-02-18T17:18:30Z

👋 @tobiasstadler Thanks a lot for your contribution!

It may take some time before we review a PR, so even if you don’t see activity for some time, it does not mean that we have forgotten about it.

Every once in a while we go through a process of prioritization, after which we are focussing on the tasks that were planned for the upcoming milestone. The prioritization status is typically reflected through the PR labels. It could be pending triage, a candidate for a future milestone, or have a target milestone set to it.

tobiasstadler · 2022-02-18T17:22:39Z

apm-agent-core/src/main/java/co/elastic/apm/agent/configuration/CoreConfiguration.java

+    private final ConfigurationOption<Boolean> spanCompressionEnabled = ConfigurationOption.booleanOption()
+        .key("span_compression_enabled")
+        .configurationCategory(CORE_CATEGORY)
+        .tags("added[1.30.0]", "internal")


Until #2083 and #2084 are done

apmmachine · 2022-02-18T17:22:49Z

💚 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

Start Time: 2022-03-03T15:04:51.610+0000
Duration: 50 min 8 sec

Test stats 🧪

Test	Results
Failed	0
Passed	2740
Skipped	16
Total	2756

💚 Flaky test report

Tests succeeded.

🤖 GitHub comments

To re-run your PR in the CI, just comment with:

/test : Re-trigger the build.
run benchmark tests : Run the benchmark tests.
run jdk compatibility tests : Run the JDK Compatibility tests.
run integration tests : Run the Agent Integration tests.
run end-to-end tests : Run the APM-ITs.
run windows tests : Build & tests on windows.
run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

felixbarny · 2022-02-21T09:19:33Z

Thanks for your PR! Note that @jackshirazi is already working on this, so please coordinate before proceeding.

@AlexanderWert has now implemented a status indicator in the corresponding issue (#1847) so that it's easier to spot if something is in progress already.

jackshirazi · 2022-02-21T10:31:45Z

@tobiasstadler please could you confirm I have the correct connection in Slack

tobiasstadler · 2022-02-21T10:43:43Z

@jackshirazi You pinged the correct person

…irazi

…irazi

apm-agent-core/src/main/java/co/elastic/apm/agent/configuration/SpanConfiguration.java

…istent span compression

apm-agent-core/src/main/java/co/elastic/apm/agent/impl/transaction/Span.java

apm-agent-core/src/test/java/co/elastic/apm/agent/impl/transaction/SpanCompressionIT.java

...ore/src/test/java/co/elastic/apm/agent/impl/transaction/SameKindCompressionStrategyTest.java

jackshirazi · 2022-03-03T13:01:53Z

/test

jackshirazi

Great work, thank you!

jackshirazi · 2022-03-03T16:13:28Z

@elasticmachine run elasticsearch-ci/docs

tobiasstadler · 2022-03-03T16:23:45Z

Thank You!

felixbarny · 2022-03-04T10:56:53Z

apm-agent-core/src/main/java/co/elastic/apm/agent/impl/transaction/Span.java

@@ -292,13 +303,146 @@ public void beforeEnd(long epochMicros) {

    @Override
    protected void afterEnd() {
-        this.tracer.endSpan(this);
+        if (transaction != null && transaction.isSpanCompressionEnabled() && parent != null) {
+            Span buffered = parent.bufferedSpan.get();


Seems like this may lead to multiple threads concurrently having access to the buffered span instance.
In my initial POC for span compression, I tried to avoid that by atomically getting and removing the buffered span or setting it to the current span if there's no buffered span.

https://github.com/felixbarny/apm-agent-java/blob/d15716baab62eaa8ff7b677da704d9c8d780d285/apm-agent-core/src/main/java/co/elastic/apm/agent/impl/transaction/AbstractSpan.java#L190-L219

Multiple threads can concurrently have access, that's okay here. It is resolved at the update or report stage which you can fully reason about here (nice when it's concurrent). There are 3 possible atomic concurrent updates, and in each case either it succeeds (true) or fails (false). The cases are complete

buffer -> null true: report buffer false: do nothing null -> this true: do nothing false: report this buffer -> this true: report buffer false: report this

Yes, concurrent access is fine here and also covered by tests.

felixbarny · 2022-03-04T10:59:59Z

apm-agent-core/src/main/java/co/elastic/apm/agent/impl/transaction/Span.java

+        return isExit() && isDiscardable() && (outcomeNotSet() || getOutcome() == Outcome.SUCCESS);
+    }
+
+    private boolean tryToCompress(Span sibling) {


Seems like this method could also be simplified when we can guarantee that we have exclusive access to both this span and the sibling. (see above linked POC)

@felixbarny I agree your solution was the more elegant one. I decided that in the high contention case (which is really when these alternatives matter) we want to avoid the backoff and retry loop, ie that under conflict it's better to drop the compression and allow the thread to proceed asap rather than maximally try to compress. So felt this solution was acceptable

note the loops in the try are getting maxes after compression has succeeded, so should not really cause contention, but we may need to review

Thanks for explaining, makes sense. I agree that the approach taken in this PR is probably the one that causes less contention. I find it a little harder to reason about and verify correctness as there's more that can happen concurrently. But from what I can tell, it seems like all the cases are handled properly.

Added (initial) support for compressing spans

94c8cbc

github-actions bot added the agent-java label Feb 18, 2022

Added elastic#2477 to the changelog

8667bfb

github-actions bot added community Issues and PRs created by the community triage labels Feb 18, 2022

tobiasstadler commented Feb 18, 2022

View reviewed changes

tobiasstadler added 4 commits February 19, 2022 07:49

Added test case for context propagation

cd2a889

It is not necessary to activate the transaction

891dc23

Report "compressed spans" as dropped

40b709f

Added assertions for span counts to compression tests

dbc556e

tobiasstadler force-pushed the fix-1847 branch from beac422 to dbc556e Compare February 21, 2022 09:11

Move compression configuration to new class as discussed with @jacksh…

147a829

…irazi

tobiasstadler commented Feb 22, 2022

View reviewed changes

apm-agent-core/src/main/java/co/elastic/apm/agent/configuration/SpanConfiguration.java Show resolved Hide resolved

tobiasstadler commented Feb 22, 2022

View reviewed changes

apm-agent-core/src/main/java/co/elastic/apm/agent/configuration/SpanConfiguration.java Show resolved Hide resolved

tobiasstadler commented Feb 22, 2022

View reviewed changes

apm-agent-core/src/main/java/co/elastic/apm/agent/configuration/SpanConfiguration.java Show resolved Hide resolved

Clear bufferedSpan when the transaction/span is recycled

a11c04f

tobiasstadler force-pushed the fix-1847 branch from 30e9427 to a11c04f Compare February 22, 2022 16:37

tobiasstadler added 2 commits February 22, 2022 17:41

Prevent NullPointerException

d595005

Cache "SpanConfiguration" when the transaction starts to provide cons…

f5c7047

…istent span compression

tobiasstadler commented Feb 22, 2022

View reviewed changes

apm-agent-core/src/main/java/co/elastic/apm/agent/impl/transaction/Span.java Show resolved Hide resolved

tobiasstadler commented Feb 22, 2022

View reviewed changes

apm-agent-core/src/main/java/co/elastic/apm/agent/impl/transaction/Span.java Outdated Show resolved Hide resolved

tobiasstadler added 3 commits February 23, 2022 17:17

Review by @jackshirazi

9303d31

Review by @jackshirazi

d879444

removed the synchronized locked

dca0fb1

jackshirazi reviewed Feb 28, 2022

View reviewed changes

apm-agent-core/src/main/java/co/elastic/apm/agent/impl/transaction/Span.java Outdated Show resolved Hide resolved

jackshirazi reviewed Feb 28, 2022

View reviewed changes

apm-agent-core/src/main/java/co/elastic/apm/agent/impl/transaction/Span.java Outdated Show resolved Hide resolved

Review by @jackshirazi

32e720b

tobiasstadler force-pushed the fix-1847 branch from fdbbda9 to 32e720b Compare February 28, 2022 16:00

jackshirazi reviewed Feb 28, 2022

View reviewed changes

apm-agent-core/src/main/java/co/elastic/apm/agent/impl/transaction/Span.java Outdated Show resolved Hide resolved

tobiasstadler added 3 commits March 1, 2022 07:26

A sibling span might have been started before the composite span

0bd7e7c

Fix NPE in switch(composite.getCompressionStrategy())

33a6909

Added tests for parallel span compression

5fff5ae

tobiasstadler commented Mar 1, 2022

View reviewed changes

apm-agent-core/src/test/java/co/elastic/apm/agent/impl/transaction/SpanCompressionIT.java Show resolved Hide resolved

tobiasstadler commented Mar 1, 2022

View reviewed changes

apm-agent-core/src/test/java/co/elastic/apm/agent/impl/transaction/SpanCompressionIT.java Show resolved Hide resolved

AlexanderWert removed the triage label Mar 1, 2022

jackshirazi reviewed Mar 3, 2022

View reviewed changes

...ore/src/test/java/co/elastic/apm/agent/impl/transaction/SameKindCompressionStrategyTest.java Show resolved Hide resolved

jackshirazi approved these changes Mar 3, 2022

View reviewed changes

jackshirazi enabled auto-merge (squash) March 3, 2022 16:13

jackshirazi merged commit e1935eb into elastic:main Mar 3, 2022

tobiasstadler deleted the fix-1847 branch March 3, 2022 16:23

felixbarny reviewed Mar 4, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added (initial) support for compressing spans #2477

Added (initial) support for compressing spans #2477

tobiasstadler commented Feb 18, 2022 •

edited by zube bot

Loading

github-actions bot commented Feb 18, 2022

tobiasstadler Feb 18, 2022

apmmachine commented Feb 18, 2022 •

edited

Loading

Build stats

Test stats 🧪

felixbarny commented Feb 21, 2022

jackshirazi commented Feb 21, 2022

tobiasstadler commented Feb 21, 2022

jackshirazi commented Mar 3, 2022

jackshirazi left a comment

jackshirazi commented Mar 3, 2022

tobiasstadler commented Mar 3, 2022

felixbarny Mar 4, 2022

jackshirazi Mar 4, 2022

tobiasstadler Mar 7, 2022

felixbarny Mar 4, 2022

jackshirazi Mar 4, 2022

jackshirazi Mar 4, 2022

felixbarny Mar 4, 2022

Added (initial) support for compressing spans #2477

Added (initial) support for compressing spans #2477

Conversation

tobiasstadler commented Feb 18, 2022 • edited by zube bot Loading

What does this PR do?

Checklist

github-actions bot commented Feb 18, 2022

Choose a reason for hiding this comment

apmmachine commented Feb 18, 2022 • edited Loading

💚 Build Succeeded

Build stats

Test stats 🧪

💚 Flaky test report

🤖 GitHub comments

felixbarny commented Feb 21, 2022

jackshirazi commented Feb 21, 2022

tobiasstadler commented Feb 21, 2022

jackshirazi commented Mar 3, 2022

jackshirazi left a comment

Choose a reason for hiding this comment

jackshirazi commented Mar 3, 2022

tobiasstadler commented Mar 3, 2022

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

Choose a reason for hiding this comment

tobiasstadler commented Feb 18, 2022 •

edited by zube bot

Loading

apmmachine commented Feb 18, 2022 •

edited

Loading