Skip to content

Issue 846 add errorid to mdc#1050

Merged
eyalkoren merged 22 commits intoelastic:masterfrom
videnkz:issue-846-add-errorid-to-mdc
Apr 1, 2020
Merged

Issue 846 add errorid to mdc#1050
eyalkoren merged 22 commits intoelastic:masterfrom
videnkz:issue-846-add-errorid-to-mdc

Conversation

@videnkz
Copy link
Contributor

@videnkz videnkz commented Feb 24, 2020 •
edited
Loading

What does this PR do?

closes #846

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

Author's Checklist

Related issues

Use cases

Screenshots
@videnkz videnkz changed the title Issue 846 add errorid to mdc Feb 24, 2020
@felixbarny
Copy link
Member

felixbarny commented Mar 6, 2020

looks really good already! Given it's titled with wip, what's left to do here?
@videnkz videnkz force-pushed the issue-846-add-errorid-to-mdc branch from 2e0ed11 to 76a0997 Compare March 10, 2020 04:35
@videnkz
Copy link
Contributor Author

videnkz commented Mar 10, 2020

looks really good already! Given it's titled with wip, what's left to do here?

Hi, I added integration test MdcActivationListenerIT that verify error.id putting to MDC context.
@videnkz videnkz changed the title wip: Issue 846 add errorid to mdc Mar 10, 2020
felixbarny
felixbarny requested changes Mar 10, 2020
View reviewed changes
videnkz and others added 4 commits March 11, 2020 15:04
@videnkz @felixbarny
Update apm-agent-plugins/apm-log-correlation-plugin/src/test/java/co/…
6a941e9 
…elastic/apm/agent/mdc/MdcActivationListenerIT.java

Co-Authored-By: Felix Barnsteiner <felixbarny@users.noreply.github.com>
@videnkz @felixbarny
Update apm-agent-plugins/apm-log-correlation-plugin/src/test/java/co/…
d09f441 
…elastic/apm/agent/mdc/MdcActivationListenerIT.java

Co-Authored-By: Felix Barnsteiner <felixbarny@users.noreply.github.com>
@videnkz @felixbarny
Update apm-agent-plugins/apm-log-correlation-plugin/src/test/java/co/…
181126b 
…elastic/apm/agent/mdc/MdcActivationListenerIT.java

Co-Authored-By: Felix Barnsteiner <felixbarny@users.noreply.github.com>
fixed according to comments
2ebcdcb
felixbarny
felixbarny reviewed Mar 12, 2020
View reviewed changes
felixbarny
felixbarny reviewed Mar 12, 2020
View reviewed changes
...m-log-correlation-plugin/src/test/java/co/elastic/apm/agent/mdc/MdcActivationListenerIT.java Outdated

private void assertMdcErrorIdIsEmpty() {
assertThat(MDC.get("error.id")).isNull();
if (org.apache.log4j.MDC.get("test") == Boolean.TRUE) {
Copy link
Member

@felixbarny felixbarny Mar 12, 2020

Choose a reason for hiding this comment

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

This if can be removed, as we're using assumptions in the test methods now. The way assumptions work is similar to assertions but instead of failing the test, they just skip the test.
Copy link
Contributor Author

@videnkz videnkz Mar 12, 2020

Choose a reason for hiding this comment

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

fixed
felixbarny
felixbarny reviewed Mar 12, 2020
View reviewed changes
...m-log-correlation-plugin/src/test/java/co/elastic/apm/agent/mdc/MdcActivationListenerIT.java Outdated

private Answer<Void> assertMdcErrorIdIsNotEmpty() {
assertThat(MDC.get("error.id")).isNotBlank();
if (org.apache.log4j.MDC.get("test") == Boolean.TRUE) {
Copy link
Member

@felixbarny felixbarny Mar 12, 2020

Choose a reason for hiding this comment

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

remove this, too
Copy link
Contributor Author

@videnkz videnkz Mar 12, 2020

Choose a reason for hiding this comment

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

removed
felixbarny
felixbarny reviewed Mar 12, 2020
View reviewed changes
Copy link
Member

@felixbarny felixbarny left a comment

Choose a reason for hiding this comment

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

Almost there 🙂

Do the tests pass now?
@videnkz
Copy link
Contributor Author

videnkz commented Mar 12, 2020

Almost there slightly_smiling_face

Do the tests pass now?

image
felixbarny
felixbarny approved these changes Mar 12, 2020
View reviewed changes
Copy link
Member

@felixbarny felixbarny left a comment

Choose a reason for hiding this comment

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

LGTM
@felixbarny felixbarny requested a review from eyalkoren March 16, 2020 10:13
eyalkoren
eyalkoren suggested changes Mar 18, 2020
View reviewed changes
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.

Thanks @kananindzya , looks good!
A few required minor changes and we're done.
@eyalkoren
Copy link
Contributor

eyalkoren commented Mar 18, 2020

@felixbarny do we have a UI issue for showing this correlation?
@eyalkoren
Copy link
Contributor

eyalkoren commented Mar 18, 2020 •
edited
Loading

@kananindzya Lastly, looking at the PR checklist - please:

  • add to changelog
  • update documentation in:
    • log-correlation.asciidoc (Step 2)
    • LoggingConfiguration (the enable_log_correlation config description). This one means you will need to test before pushing, as the asciidoc needs to get updated this way
    • supported-technologies.asciidoc (the Logging Frameworks section).

Thanks!
@videnkz @eyalkoren
Update apm-agent-core/src/main/java/co/elastic/apm/agent/impl/Elastic…
a98cddf 
…ApmTracer.java

Co-Authored-By: eyalkoren <41850454+eyalkoren@users.noreply.github.com>
@felixbarny
Copy link
Member

felixbarny commented Mar 31, 2020

I've created one: elastic/kibana#61952
@videnkz
Copy link
Contributor Author

videnkz commented Mar 31, 2020

@kananindzya Lastly, looking at the PR checklist - please:

* add to changelog

* update documentation in:
  
  * log-correlation.asciidoc (Step 2)
  * `LoggingConfiguration` (the `enable_log_correlation` config description). This one means you will need to test before pushing, as the `asciidoc` needs to get updated this way
  * supported-technologies.asciidoc (the Logging Frameworks section).

Thanks!

added
@eyalkoren
Copy link
Contributor

eyalkoren commented Apr 1, 2020

run elasticsearch-ci/docs
@eyalkoren
Copy link
Contributor

eyalkoren commented Apr 1, 2020

@kananindzya looks awesome!!
There were some fixes required in the documentation stuff, please merge my PR.
In addition, reviewing this revealed that we actually have a gap with log4j2 where we don't actually capture error when an error message is logged (see #1116 )
@videnkz
Copy link
Contributor Author

videnkz commented Apr 1, 2020

@kananindzya looks awesome!!
There were some fixes required in the documentation stuff, please merge my PR.
In addition, reviewing this revealed that we actually have a gap with log4j2 where we don't actually capture error when an error message is logged (see #1116 )

Great! Merged!
@eyalkoren
Copy link
Contributor

eyalkoren commented Apr 1, 2020

run elasticsearch-ci/docs
@codecov-io
Copy link

codecov-io commented Apr 1, 2020

Codecov Report

Merging #1050 into master will decrease coverage by 0.17%.
The diff coverage is 47.22%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #1050      +/-   ##
============================================
- Coverage     60.19%   60.02%   -0.18%     
  Complexity       87       87              
============================================
  Files           327      327              
  Lines         14844    14799      -45     
  Branches       2081     2067      -14     
============================================
- Hits           8936     8883      -53     
- Misses         5313     5315       +2     
- Partials        595      601       +6
Impacted Files Coverage Δ Complexity Δ
...lastic/apm/agent/logging/LoggingConfiguration.java 92.30% <ø> (ø) 0.00 <0.00> (ø)
...nt/plugin/api/CaptureExceptionInstrumentation.java 57.14% <0.00%> (ø) 0.00 <0.00> (ø)
...agent/plugin/api/ElasticApmApiInstrumentation.java 40.54% <0.00%> (ø) 0.00 <0.00> (ø)
.../error/logging/AbstractLoggingInstrumentation.java 45.83% <0.00%> (-4.17%) 0.00 <0.00> (ø)
...pm/agent/profiler/ProfilingActivationListener.java 57.14% <0.00%> (ø) 0.00 <0.00> (ø)
...o/elastic/apm/agent/mdc/MdcActivationListener.java 86.11% <33.33%> (-5.07%) 0.00 <0.00> (ø)
.../co/elastic/apm/agent/impl/error/ErrorCapture.java 87.34% <37.50%> (-5.81%) 0.00 <0.00> (ø)
...va/co/elastic/apm/agent/impl/ElasticApmTracer.java 71.14% <83.33%> (+0.20%) 0.00 <0.00> (ø)
...apm/agent/impl/transaction/TraceContextHolder.java 56.25% <100.00%> (+1.14%) 0.00 <0.00> (ø)
...main/java/co/elastic/apm/agent/metrics/Labels.java 73.87% <0.00%> (-4.49%) 0.00% <0.00%> (ø%)
... and 3 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 3769443...fc70b91. Read the comment docs.
eyalkoren
eyalkoren approved these changes Apr 1, 2020
View reviewed changes
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.

Well done @kananindzya!! 🎉
@eyalkoren eyalkoren merged commit b61ba4a into elastic:master Apr 1, 2020
@zube zube bot removed the [zube]: Done label Oct 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

4 participants

@videnkz @felixbarny @eyalkoren @codecov-io