Skip to content
This repository has been archived by the owner on Jul 1, 2022. It is now read-only.

Add MDCScopeManager for correlating logs with trace context #718

Merged
merged 12 commits into from Jun 27, 2020
Merged

Add MDCScopeManager for correlating logs with trace context #718

merged 12 commits into from Jun 27, 2020

Conversation

avimas
Copy link
Contributor

@avimas avimas commented Jun 23, 2020

HI

I am new to Jaeger and this is an initial solution to #715 I'd like to know if this approach (warping the scope manager) is an acceptable solution.

Which problem is this PR solving?

Short description of the changes

  • adding new MDC scope manager.

avim added 3 commits June 23, 2020 17:00
Signed-off-by: avim <avi.maslati@forescout.com>
Signed-off-by: avim <avi.maslati@forescout.com>
Signed-off-by: avim <avi.maslati@forescout.com>
@codecov
Copy link

codecov bot commented Jun 23, 2020

Codecov Report

Merging #718 into master will decrease coverage by 1.02%.
The diff coverage is 95.34%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #718      +/-   ##
============================================
- Coverage     89.78%   88.76%   -1.03%     
- Complexity      590      596       +6     
============================================
  Files            70       73       +3     
  Lines          2164     2242      +78     
  Branches        284      289       +5     
============================================
+ Hits           1943     1990      +47     
- Misses          132      159      +27     
- Partials         89       93       +4     
Impacted Files Coverage Δ Complexity Δ
...ava/io/jaegertracing/internal/MDCScopeManager.java 95.34% <95.34%> (ø) 3.00 <3.00> (?)
...rtracing/internal/reporters/CompositeReporter.java 71.42% <0.00%> (-28.58%) 6.00% <0.00%> (-1.00%)
...a/io/jaegertracing/internal/clock/SystemClock.java 75.00% <0.00%> (-25.00%) 6.00% <0.00%> (+2.00%) ⬇️
...gertracing/internal/reporters/LoggingReporter.java 81.81% <0.00%> (-9.10%) 4.00% <0.00%> (-1.00%)
...egertracing/internal/propagation/TextMapCodec.java 92.72% <0.00%> (-1.82%) 35.00% <0.00%> (-1.00%)
...gertracing/internal/clock/MicrosAccurateClock.java 0.00% <0.00%> (ø) 0.00% <0.00%> (?%)
...ertracing/internal/clock/MillisAccurrateClock.java 100.00% <0.00%> (ø) 4.00% <0.00%> (?%)
...n/java/io/jaegertracing/internal/JaegerTracer.java 89.22% <0.00%> (+0.14%) 27.00% <0.00%> (ø%)

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 5831d9e...24ee79c. Read the comment docs.

Copy link
Member

@yurishkuro yurishkuro left a comment

Choose a reason for hiding this comment

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

I thing it's a reasonable addition.

avim added 2 commits June 24, 2020 15:00
Signed-off-by: avim <avi.maslati@forescout.com>
Signed-off-by: avim <avi.maslati@forescout.com>
Copy link
Member

@pavolloffay pavolloffay left a comment

Choose a reason for hiding this comment

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

Could you add a test with two spans created? The second span would be a child of the first one. Then after the second span is closed assert that MDC contains context from the parent.

@avimas avimas requested a review from pavolloffay June 25, 2020 07:45
Signed-off-by: avim <avi.maslati@forescout.com>
@avimas avimas changed the title Support MDC initial Support MDC Jun 25, 2020
Copy link
Member

@pavolloffay pavolloffay left a comment

Choose a reason for hiding this comment

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

small nits, otherwise LGTM

@avimas avimas requested a review from pavolloffay June 25, 2020 12:37
avim added 2 commits June 25, 2020 15:39
Signed-off-by: avim <avi.maslati@forescout.com>
Signed-off-by: avim <avi.maslati@forescout.com>
Copy link
Member

@yurishkuro yurishkuro left a comment

Choose a reason for hiding this comment

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

lgtm

@yurishkuro yurishkuro changed the title Support MDC Add MDCScopeManager for correlating logs with trace context Jun 25, 2020
Signed-off-by: avim <avi.maslati@forescout.com>
@yurishkuro
Copy link
Member

@pavolloffay do we have a way to disable style check for capital letters?

Abbreviation in name 'MDCScopeManager' must contain no more than '2' consecutive capital letters.

@pavolloffay
Copy link
Member

I don't know

avim added 2 commits June 26, 2020 17:11
Signed-off-by: avim <avi.maslati@forescout.com>
Signed-off-by: avim <avi.maslati@forescout.com>
@avimas
Copy link
Contributor Author

avimas commented Jun 26, 2020

@yurishkuro changed the style check config to allow 4 consecutive letters, the build fails in codecov check now
anything I can do?

@@ -162,7 +162,7 @@
</module>
<module name="AbbreviationAsWordInName">
<property name="ignoreFinal" value="false"/>
<property name="allowedAbbreviationLength" value="1"/>
<property name="allowedAbbreviationLength" value="3"/>
Copy link
Member

Choose a reason for hiding this comment

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

I was thinking of whether we can disable the rule specifically for the two MDC classes, not globally for the project, do you know if it's possible?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@yurishkuro , Yes I added these classes to the suppressions.xml file
the build still fails in codecov I don't know why

escape only MDC classes

Signed-off-by: avim <avi.maslati@forescout.com>
Copy link
Member

@yurishkuro yurishkuro left a comment

Choose a reason for hiding this comment

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

Thanks!

I am going to merge this, but could you please create another PR and add a section to the README about the new functionality?

@yurishkuro yurishkuro merged commit 74f4aa2 into jaegertracing:master Jun 27, 2020
@avimas avimas deleted the mdc-integration branch June 28, 2020 08:44
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add trace IDs into std logging (MDC)
3 participants