Skip to content

Fix parent/child issues with inferred spans#1117

Merged
felixbarny merged 21 commits intoelastic:masterfrom
felixbarny:inferred-spans-successor-ids
May 12, 2020
Merged

Fix parent/child issues with inferred spans#1117
felixbarny merged 21 commits intoelastic:masterfrom
felixbarny:inferred-spans-successor-ids

Conversation

@felixbarny
Copy link
Member

@felixbarny felixbarny commented Apr 2, 2020

Inferred spans can track a list of child or successor ids
With transitive reduction, the UI can then make regular spans a child of inferred spans

What does this PR do?

closes #1112

Checklist

  • [ ] 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 CHANGELOG.asciidoc
  • [ ] 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? How did you make sure that old, non-supported versions are not instrumented by accident?
Inferred spans can track a list of child or successor ids
With transitive reduction, the UI can then make regular spans a child of inferred spans
This capacity seems to be the most effective in the benchmarks
But it's going to depend a lot on the application
16 still seems like a good default.
It grows much quicker if the actual capacity is high
but still does not seem wasteful for lower capacity needs.
It's also the default for lists in the JDK
- Add changelog
- Adjust known issues documentation
@felixbarny felixbarny requested a review from eyalkoren April 24, 2020 15:11
@felixbarny felixbarny marked this pull request as ready for review April 24, 2020 15:12
@codecov-io
Copy link

codecov-io commented Apr 26, 2020

Codecov Report

Merging #1117 into master will decrease coverage by 12.67%.
The diff coverage is n/a.

Impacted file tree graph

@@              Coverage Diff              @@
##             master    #1117       +/-   ##
=============================================
- Coverage     59.69%   47.02%   -12.68%     
- Complexity       83      840      +757     
=============================================
  Files           328      106      -222     
  Lines         14997     5854     -9143     
  Branches       2086      990     -1096     
=============================================
- Hits           8953     2753     -6200     
+ Misses         5433     2896     -2537     
+ Partials        611      205      -406     
Impacted Files Coverage Δ Complexity Δ
...m/agent/util/DependencyInjectingServiceLoader.java
...c/apm/agent/context/AbstractLifecycleListener.java
.../apm/agent/okhttp/OkHttpClientInstrumentation.java
...t/kafka/helper/ConsumerRecordsIterableWrapper.java
...astic/apm/agent/impl/sampling/ConstantSampler.java
.../main/java/co/elastic/apm/agent/jmx/JmxMetric.java
...esttemplate/SpringRestTemplateInstrumentation.java
...agent/plugin/api/ElasticApmApiInstrumentation.java
...agent/httpclient/helper/RequestHeaderAccessor.java
...sticsearchRestClientInstrumentationHelperImpl.java
... and 414 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 73962e6...ca6f7ae. Read the comment docs.

Copy link
Contributor

@eyalkoren eyalkoren left a comment

Choose a reason for hiding this comment

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

🤯

@ghost
Copy link

ghost commented Apr 28, 2020

💚 Build Succeeded

Pipeline View Test View Changes Artifacts preview stats

Expand to view the summary

Build stats

Test stats 🧪

Test Results
Failed 0
Passed 1270
Skipped 12
Total 1282

{"1", 13},
{" a", 11},
{" b", 2},
{" 2", 6},
Copy link
Contributor

Choose a reason for hiding this comment

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

How come 2 is not a child of a?

Copy link
Member Author

Choose a reason for hiding this comment

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

Wow, good catch! From now on, I call you the eagle eye. Or maybe Eyeal 🤔

Submitting fix shortly that searches for common ancestors, instead of always adding the child_ids on the top of the stack

@ghost
Copy link

ghost commented May 8, 2020

💚 Build Succeeded

Pipeline View Test View Changes Artifacts preview stats

Expand to view the summary

Build stats

Test stats 🧪

Test Results
Failed 0
Passed 1297
Skipped 12
Total 1309

- Adds internal profiling_inferred_spans_backup_diagnostic_files option
- Adds SamplingProfilerReplay to diagnose issues when creating inferred spans
@felixbarny felixbarny merged commit 28e6f45 into elastic:master May 12, 2020
@felixbarny felixbarny deleted the inferred-spans-successor-ids branch May 12, 2020 10:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

3 participants