Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow using wildcards with RequestContextExportingAppender #1742

Merged
merged 5 commits into from May 9, 2019

Conversation

Projects
None yet
5 participants
@masonshin
Copy link
Collaborator

commented Apr 25, 2019

For #489

I applied a wildcard * pattern about builtin properties. We can also adopt a wildcard about headers or attrs. But it can be caused some overhead to by logging.
I also specified to document that the wildcard applies only to the builtin property.

@masonshin masonshin requested review from hyangtack, minwoox and trustin as code owners Apr 25, 2019

@CLAassistant

This comment has been minimized.

Copy link

commented Apr 25, 2019

CLA assistant check
All committers have signed the CLA.

@codecov

This comment has been minimized.

Copy link

commented Apr 25, 2019

Codecov Report

Merging #1742 into master will decrease coverage by 1.13%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #1742      +/-   ##
============================================
- Coverage     72.93%   71.79%   -1.14%     
- Complexity     8153     8418     +265     
============================================
  Files           736      760      +24     
  Lines         32465    33787    +1322     
  Branches       3989     4159     +170     
============================================
+ Hits          23678    24259     +581     
- Misses         6757     7421     +664     
- Partials       2030     2107      +77
Impacted Files Coverage Δ Complexity Δ
...necorp/armeria/common/logback/BuiltInProperty.java 100% <100%> (ø) 5 <2> (+1) ⬆️
.../common/logback/RequestContextExporterBuilder.java 84.28% <100%> (+0.46%) 21 <0> (+1) ⬆️
...ringframework/boot/minimal/HelloConfiguration.java 0% <0%> (-100%) 0% <0%> (-3%)
...e/springframework/boot/tomcat/HelloController.java 0% <0%> (-100%) 0% <0%> (-3%)
...eria/client/encoding/GzipStreamDecoderFactory.java 0% <0%> (-100%) 0% <0%> (-3%)
...a/client/encoding/DeflateStreamDecoderFactory.java 0% <0%> (-100%) 0% <0%> (-3%)
...pringframework/boot/tomcat/HelloConfiguration.java 0% <0%> (-100%) 0% <0%> (-6%)
...ia/client/limit/ConcurrencyLimitingHttpClient.java 0% <0%> (-100%) 0% <0%> (-7%)
...rp/armeria/common/HttpHeadersJsonDeserializer.java 0% <0%> (-92%) 0% <0%> (-8%)
...rp/armeria/client/encoding/HttpDecodingClient.java 0% <0%> (-86.67%) 0% <0%> (-5%)
... and 196 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 35cb85f...e8cfd3d. Read the comment docs.

@trustin trustin added the new-feature label Apr 25, 2019

@trustin

This comment has been minimized.

Copy link
Member

commented May 2, 2019

Gentle ping, @masonshin 😉

@masonshin masonshin requested a review from trustin May 3, 2019

@masonshin

This comment has been minimized.

Copy link
Collaborator Author

commented May 3, 2019

@trustin Sorry too late update. PTAL once again. :)

@trustin

trustin approved these changes May 8, 2019

Copy link
Member

left a comment

Great job! 👍

@trustin trustin added this to the 0.85.0 milestone May 8, 2019

return;
}

if (mdcKey.contains(BuiltInProperty.WILDCARD_STR)) {
return;

This comment has been minimized.

Copy link
@minwoox

minwoox May 9, 2019

Member

Just wondering if we need to log or throw an exception when the key contains * and there are no properties found for that.

This comment has been minimized.

Copy link
@masonshin

masonshin May 9, 2019

Author Collaborator

Since the document tells explicitly that it only supports built-in properties, it is better to tell it clearly if no matches are found.

This comment has been minimized.

Copy link
@minwoox

minwoox May 9, 2019

Member

@masonshin Could you revert it, please? 🙇
Have thought about it again and we can skip this case silently because it means including the properties matched by the pattern. (i.e there could be no match and we can skip to the next one.)

This comment has been minimized.

Copy link
@masonshin

masonshin May 9, 2019

Author Collaborator

I agree. Let me revert it.

masonshin added some commits May 9, 2019

@minwoox

minwoox approved these changes May 9, 2019

Copy link
Member

left a comment

Forgot to approve. 😉
Great job!!

@trustin trustin merged commit 5162350 into line:master May 9, 2019

5 checks passed

Travis CI - Pull Request Build Passed
Details
codecov/patch 100% of diff hit (target 72.93%)
Details
codecov/project Absolute coverage decreased by -1.13% but relative coverage increased by +27.06% compared to 35cb85f
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
license/cla Contributor License Agreement is signed.
Details
@trustin

This comment has been minimized.

Copy link
Member

commented May 9, 2019

Thank you, @masonshin!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.