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

Adding correlationId from 4financeIT micro-infra-spring #1

Merged

Conversation

marcingrzejszczak
Copy link
Contributor

e4financeIT CorrelationID PR

What I have done:

  • migrated CorrelationID filter (tested)
  • added interceptor to the registered RestTemplate to add correlationID (tested)
  • added autoconfiguration for correlationId (tested)
  • added aspect that wraps executions with correlationdId (tested)
  • migrated possibility to remove URI patterns from correlation id setting (tested)
  • created a separate module for correlationid (tested)
  • created a separate module for rest-template configuration setting (tested)
  • added copyrights
  • add authors

What I had to do:

  • <spring-cloud.version>1.0.1.RELEASE</spring-cloud.version> because the SNAPSHOT one didn't work for Zuul and Hystrix

What has to be done:

  • [] move camel support?
  • [] move reactor support?

@marcingrzejszczak marcingrzejszczak changed the title Adding correlationId Adding correlationId from 4financeIT micro-infra-spring May 29, 2015
@spencergibb
Copy link
Member

I can work on guava for zipkin since it's part of their api. I'd put your correlation stuff in another module, maybe spring-cloud-sleuth-correlation? That's easy enough to change anyway. I'm not sure about if we want to put camel support here, might be something you guys continue to maintain, but I'll get @dsyer's opinion on that.

@marcingrzejszczak
Copy link
Contributor Author

I had to ignore some zipkin code cause I wasn't able to build the app.

I've extracted RestTemplate bean setup to a new module spring-cloud-sleuth-core. I need it for tests if the ClientHttpRequestInterceptor in the correlation module is properly set and is operational. It's also used in the zipkin module.

I don't know if the property names are ok - so please check them out :)

@spencergibb
Copy link
Member

@marcingrzejszczak if you rebase against master you should be able to un-comment the zipkin code and it should build again

@marcingrzejszczak
Copy link
Contributor Author

Done :)

Unfortunately after uncommenting the lines the code doesn't compile - maybe I've done sth wrong :/

@marcingrzejszczak marcingrzejszczak force-pushed the adding-correlation-id branch 2 times, most recently from b274890 to c0fc585 Compare June 1, 2015 22:33
@marcingrzejszczak
Copy link
Contributor Author

Ok Fixed that. BTW I can't locally build this. ALl the time I have issues with Guava.

@spencergibb
Copy link
Member

@marcingrzejszczak I can build and travis can build. Maybe force update your maven cache when you build? mvn -U install. I had to make a change to spring-cloud-build to allow exceptions to the guava rule. You probably have an old copy that still blindly blocks it.

@marcingrzejszczak
Copy link
Contributor Author

Yeah I see that the builds pass - I have to have something done wrong on my side.

Anyways - are we ready to merge or do you want me to do sth else? If not then I can squash the commits

@spencergibb
Copy link
Member

If you can build, I think I'm ready to merge

Below you can find people who helped to create this solution

Tomasz Nurkiewicz <tomasz.nurkiewicz@4finance.com>
Marcin Zajaczkowski <marcin.zajaczkowski@4finance.com>
Kamil Szymanski <kamil.szymanski@4finance.com>
Michal Chmielarz <michal.chmielarz@4finance.com>
Marcin Grzejszczak <marcin.grzejszczak@4finance.com>
Jakub Nabdralik <jakub.nabrdalik@4finance.com>
Urszula Choromanska <urszula.choromanska@4finance.com>
Tomasz Dziurko <tomasz.dziurko@4finance.com>
Tomasz Szymanski <tomasz.szymanski@4finance.com>
Adam Chudzik <adam.chudzik@4finance.com>
@marcingrzejszczak
Copy link
Contributor Author

I still couldn't build it locally but Travis says ok :)

@marcingrzejszczak
Copy link
Contributor Author

Once you merge - can you Tweet about us again :) ?

@marcingrzejszczak
Copy link
Contributor Author

And (if it's not a problem) you could mention about 4financeIT in Spring Weekly ;)

spencergibb added a commit that referenced this pull request Jun 4, 2015
Adding correlationId from 4financeIT micro-infra-spring
@spencergibb spencergibb merged commit 487d7c0 into spring-cloud:master Jun 4, 2015
@spencergibb
Copy link
Member

Merged, I'll have to defer to @joshlong as to what goes into Spring Weekly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants