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

Making Logging a separate module outside of autoconfiguration #1455

Merged
merged 4 commits into from
Mar 11, 2019

Conversation

meltsufin
Copy link
Contributor

Fixes: #1448.

@meltsufin
Copy link
Contributor Author

@artembilan I decided against the "stackdriver" module because there is nothing useful to put there for the trace stuff, only logging.

@codecov-io
Copy link

codecov-io commented Feb 7, 2019

Codecov Report

Merging #1455 into master will increase coverage by 7.29%.
The diff coverage is 62.9%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #1455      +/-   ##
============================================
+ Coverage     69.15%   76.45%   +7.29%     
+ Complexity     1520     1387     -133     
============================================
  Files           215      142      -73     
  Lines          5898     4698    -1200     
  Branches        600      536      -64     
============================================
- Hits           4079     3592     -487     
+ Misses         1567      861     -706     
+ Partials        252      245       -7
Flag Coverage Δ Complexity Δ
#integration ? ?
#unittests 76.45% <62.9%> (+7.29%) 1387 <25> (-133) ⬇️
Impacted Files Coverage Δ Complexity Δ
...k/cloud/gcp/logging/StackdriverTraceConstants.java 100% <100%> (ø) 1 <1> (?)
...gcp/logging/extractors/XCloudTraceIdExtractor.java 100% <100%> (ø) 4 <4> (?)
...work/cloud/gcp/logging/TraceIdLoggingEnhancer.java 28.57% <28.57%> (ø) 4 <4> (?)
...ework/cloud/gcp/logging/StackdriverJsonLayout.java 64% <64%> (ø) 11 <11> (?)
...ork/cloud/gcp/logging/LoggingWebMvcConfigurer.java 71.42% <71.42%> (ø) 2 <2> (?)
...d/gcp/logging/TraceIdLoggingWebMvcInterceptor.java 81.81% <81.81%> (ø) 3 <3> (?)
...atastore/core/mapping/DatastoreMappingContext.java 92% <0%> (-8%) 11% <0%> (+4%)
...nner/core/convert/StructPropertyValueProvider.java 88.57% <0%> (-5.37%) 13% <0%> (ø)
...re/core/mapping/DatastorePersistentEntityImpl.java 93.61% <0%> (-3.45%) 44% <0%> (+25%)
...store/repository/query/AbstractDatastoreQuery.java 66.66% <0%> (-3.34%) 5% <0%> (ø)
... and 130 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 07529ad...5591d38. Read the comment docs.

Copy link
Contributor

@artembilan artembilan left a comment

Choose a reason for hiding this comment

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

I have only one but big concern to this change: it is breaking one.

In other my projects we try to keep the functionality as much as possible until the next major version, where we really can allow ourselves a luxury of removal.

Not sure what is the breaking changes policy in Spring Cloud (/CC @spencergibb ), but I would keep the current classes as @Deprecated at least until the next 1.3 version. This old classes may just extend new ones.
As well as that logback-appender.xml should stay in the autoconfigure as well. For the same reason.
My point is that I should be able to upgrade my project to your new version and everything works well with the gain that I know can use your new features.

The deprecation still let me to work, but I will know that your are going to remove it in the future, so I need to tale appropriate actions to mitigate an issue.

Not sure how else to explain this, but we have to keep in mind potential breaking changes already since releasing version 1.0.

I can't review all the PR in this project, but that would be great that your team would try to stay away from breaking changes as much as possible.

Thanks for understanding.

@meltsufin
Copy link
Contributor Author

@artembilan I see your point. I did expect this to come up because there are some serious breaking changes, the most important of which is the path change to logback config files. I am, however, curious about the breaking changes policy. I have seen some breakage in minor versions of Spring like from Boot 2.0 to 2.1 and Framework 5.0 to 5.1, and even more so in Spring Cloud projects.
I'll try to make this a bit more backwards compatible. Perhaps we can import the logging configs from one file to the other.

@artembilan
Copy link
Contributor

I'm not sure what you mean about Spring Framework, but we really had some breaking changes in Spring Boot from 2.0 to 2.1: https://github.com/spring-projects/spring-boot/wiki/Spring-Boot-2.1-Release-Notes
We had a big team discussion on the matter and those changes were really reasonable because previous behavior has just hidden an issue from us.

But that's fully different story and should be revised privately if that.

Anyway let's just don't make this breaking changes story like blaming each other! Let's try to keep our code from the breakage as much as possible.
This way we will eliminate many questions and issues from community.

I think it won't hurt to keep content of those logging configs as identical. Doesn't look like we change it too often...

@meltsufin
Copy link
Contributor Author

@artembilan I followed your suggestion to also keep the original classes and files as deprecated. Please take another look.

Copy link
Contributor

@artembilan artembilan left a comment

Choose a reason for hiding this comment

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

Oh! Forgot to mention: please, update Copyright for all the affected classes.

There is a failure on Travis, but i don't believe it is related to your changes: we have a bunch of failing CI plans after some recent changes. I guess might be as a fact of upgrade to the latest Spring Cloud.

@meltsufin meltsufin merged commit b9fd85d into master Mar 11, 2019
@meltsufin meltsufin deleted the logging-module branch March 11, 2019 18:16
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Development

Successfully merging this pull request may close these issues.

3 participants