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

SQS Messaging Autoconfig #1218

Merged

Conversation

@devinsba
Copy link
Contributor

commented Feb 22, 2019

partially addressed #1217

Attempts to make spring-messaging generic component for use elsewhere TracingMethodMessageHandlerAdapter. I'm still thinking about how to abstract/wrap the existing classes so code doesn't have to me copied from the Impl on the other side

@devinsba

This comment has been minimized.

Copy link
Contributor Author

commented Feb 26, 2019

@adriancole when you have a few minutes I'd appreciate feedback on the messaging model here. I'm not that familiar with it and our examples are not the most straightforward (rabbit/kafka)

Copy link
Contributor

left a comment

I have an alternate suggestion for SQS which will make the model look exact same as rabbit (which makes sense as both are remote brokers)

@devinsba devinsba marked this pull request as ready for review Mar 3, 2019
@devinsba

This comment has been minimized.

Copy link
Contributor Author

commented Mar 3, 2019

@marcingrzejszczak I'm not sure why the test that is failing is failing. It's not in the stuff I modified so I am a bit lost. Maybe my test is not cleaning up after itself completely? Not sure

@@ -82,6 +82,11 @@
<groupId>org.springframework.cloud</groupId>
<artifactId>spring-cloud-commons</artifactId>
</dependency>
<dependency>
<groupId>org.springframework.cloud</groupId>
<artifactId>spring-cloud-aws-messaging</artifactId>

This comment has been minimized.

Copy link
@devinsba

devinsba Mar 3, 2019

Author Contributor

Not sure if I should bring in the starter of just the code?

This comment has been minimized.

Copy link
@marcingrzejszczak

marcingrzejszczak Mar 4, 2019

Contributor

Concrete dep is ok.

@marcingrzejszczak

This comment has been minimized.

Copy link
Contributor

commented Mar 4, 2019

@marcingrzejszczak I'm not sure why the test that is failing is failing. It's not in the stuff I modified so I am a bit lost. Maybe my test is not cleaning up after itself completely? Not sure

Flakey test 😭

@marcingrzejszczak

This comment has been minimized.

Copy link
Contributor

commented Mar 4, 2019

@devinsba if you rebase against master, you'll see that this test is ignored :| It's been flakey for some time so I've decided not to run it (and the problem is gone ;) )

@devinsba

This comment has been minimized.

Copy link
Contributor Author

commented Mar 4, 2019

I was going to rebase to squash all the checkstyle commits anyway so will do

@devinsba devinsba force-pushed the devinsba:sqs-autoconfiguration branch from 91afa56 to 585c962 Mar 4, 2019
@codecov-io

This comment has been minimized.

Copy link

commented Mar 4, 2019

Codecov Report

Merging #1218 into master will decrease coverage by 0.5%.
The diff coverage is 37.25%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #1218      +/-   ##
============================================
- Coverage     69.39%   68.89%   -0.51%     
- Complexity      775      777       +2     
============================================
  Files           143      145       +2     
  Lines          3663     3716      +53     
  Branches        401      407       +6     
============================================
+ Hits           2542     2560      +18     
- Misses          889      924      +35     
  Partials        232      232
Impacted Files Coverage Δ Complexity Δ
...aging/TraceSpringIntegrationAutoConfiguration.java 100% <ø> (ø) 3 <0> (-2) ⬇️
...ssaging/TraceSpringMessagingAutoConfiguration.java 100% <100%> (ø) 3 <3> (?)
.../messaging/TracingMethodMessageHandlerAdapter.java 15.15% <16.12%> (ø) 1 <1> (?)
...ent/messaging/TraceMessagingAutoConfiguration.java 48.41% <64.7%> (+1.68%) 1 <0> (ø) ⬇️
...ing/TracingConnectionFactoryBeanPostProcessor.java 63.93% <0%> (-8.2%) 6% <0%> (ø)
...jms/config/TracingJmsListenerEndpointRegistry.java 67.81% <0%> (+6.89%) 17% <0%> (ø) ⬇️

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 f9e029a...585c962. Read the comment docs.

@codecov-io

This comment has been minimized.

Copy link

commented Mar 4, 2019

Codecov Report

Merging #1218 into master will increase coverage by 0.32%.
The diff coverage is 85.45%.

Impacted file tree graph

@@             Coverage Diff             @@
##             master   #1218      +/-   ##
===========================================
+ Coverage     58.07%   58.4%   +0.32%     
- Complexity      802     808       +6     
===========================================
  Files           151     153       +2     
  Lines          4308    4361      +53     
  Branches        469     475       +6     
===========================================
+ Hits           2502    2547      +45     
- Misses         1592    1596       +4     
- Partials        214     218       +4
Impacted Files Coverage Δ Complexity Δ
...aging/TraceSpringIntegrationAutoConfiguration.java 100% <ø> (ø) 3 <0> (-2) ⬇️
...ssaging/TraceSpringMessagingAutoConfiguration.java 100% <100%> (ø) 3 <3> (?)
...ent/messaging/TraceMessagingAutoConfiguration.java 39.71% <84.21%> (+6.92%) 1 <0> (ø) ⬇️
.../messaging/TracingMethodMessageHandlerAdapter.java 84.84% <84.84%> (ø) 5 <5> (?)

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 9dbf99c...6c33937. Read the comment docs.

Copy link
Contributor

left a comment

Minor suggestions. Also I think that the docs are missing, right?

@devinsba

This comment has been minimized.

Copy link
Contributor Author

commented Mar 4, 2019

Yup, no docs yet. Only got it to a point I was confident over the weekend

@devinsba

This comment has been minimized.

Copy link
Contributor Author

commented Mar 4, 2019

Found a large bug, please don't merge

@devinsba

This comment has been minimized.

Copy link
Contributor Author

commented Mar 5, 2019

So it turns out it was only a bug in my slightly different internal impl for spring 4 (not boot). @marcingrzejszczak, @adriancole, @shakuzen please review when you have time

Copy link
Contributor

left a comment

data suggestions but even if not, looks good

private TraceContext.Extractor<MessageHeaderAccessor> extractor;


TracingMethodMessageHandlerAdapter(Tracing tracing,

This comment has been minimized.

Copy link
@devinsba

devinsba Mar 8, 2019

Author Contributor

Having read through more of the messaging code I'm not sure how reusable this would need to be. It seems most other things will be covered by the existing spring-integration style instrumentation. I'd love to get input from maybe someone who works on that project (spring-messaging/spring-integration) on whether maybe the SQS messaging is a special case

This comment has been minimized.

Copy link
@devinsba

devinsba Oct 2, 2019

Author Contributor

Looking through the code I can find, this does seem to become useful for @MessageMapping and @SubscribeMapping which don't look to be covered by the current websockets instrumentation so I think it is useful to leave this here

@devinsba devinsba force-pushed the devinsba:sqs-autoconfiguration branch 2 times, most recently from 7a620da to 0fd6f99 Mar 8, 2019
@marcingrzejszczak

This comment has been minimized.

Copy link
Contributor

commented Mar 21, 2019

Hey @devinsba, how is it going with this PR? Just FYI the build will be failing ATM due to issues with HATEOAS

@marcingrzejszczak marcingrzejszczak added this to the 2.2.0.M1 milestone Mar 29, 2019
@marcingrzejszczak marcingrzejszczak added this to In progress in Hoxton.M1 via automation Mar 29, 2019
@marcingrzejszczak

This comment has been minimized.

Copy link
Contributor

commented Mar 29, 2019

Hey @devinsba , I've fixed the merge conflicts. What do we do about this? :P

@spencergibb spencergibb removed this from In progress in Hoxton.M1 Jul 3, 2019
@spencergibb spencergibb added this to In progress in Hoxton.M2 via automation Jul 3, 2019
@spencergibb spencergibb removed this from the 2.2.0.M1 milestone Jul 3, 2019
@spencergibb spencergibb added this to In progress in Hoxton.M3 via automation Aug 16, 2019
@spencergibb spencergibb removed this from In progress in Hoxton.M2 Aug 16, 2019
@neoshadybeat

This comment has been minimized.

Copy link

commented Sep 3, 2019

Any updates about this PR?

@devinsba devinsba force-pushed the devinsba:sqs-autoconfiguration branch from cd607d6 to cf15c09 Oct 2, 2019
@devinsba devinsba force-pushed the devinsba:sqs-autoconfiguration branch from cf15c09 to 8644e9e Oct 2, 2019
@devinsba devinsba requested a review from marcingrzejszczak Oct 2, 2019
@marcingrzejszczak marcingrzejszczak added this to the 2.2.0.M3 milestone Oct 3, 2019
@marcingrzejszczak marcingrzejszczak merged commit dc82c5c into spring-cloud:master Oct 3, 2019
4 checks passed
4 checks passed
ci/circleci Your tests passed on CircleCI!
Details
ci/pivotal-cla Thank you for signing the Contributor License Agreement!
Details
codecov/patch 85.45% of diff hit (target 58.07%)
Details
codecov/project 58.4% (+0.32%) compared to 9dbf99c
Details
Hoxton.M3 automation moved this from In progress to Done Oct 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Hoxton.M3
  
Done
8 participants
You can’t perform that action at this time.