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

Add Observation implementation #1438

Open
wants to merge 15 commits into
base: main
Choose a base branch
from

Conversation

johkin
Copy link

@johkin johkin commented Nov 9, 2024

Initial implementation of observation for spring-ws.

Please feel free to comment and provide feedback.

#1094

@johkin
Copy link
Author

johkin commented Nov 9, 2024

Ping @corneil

@corneil
Copy link
Contributor

corneil commented Nov 11, 2024

@johkin Thanks for this great contribution.
I would suggest adding something like: Assert.notNull(observation, "Expected observation in messageContext"); at appropriate places.

@corneil corneil self-requested a review November 11, 2024 10:51
Changed name of key "localname" to "localpart"
Removed superfluos method implementations
Modified implementation of ContextualName

spring-projects#1094
Copy link
Member

@shakuzen shakuzen left a comment

Choose a reason for hiding this comment

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

Thank you for the work on this. It's a very complete pull request. I left some feedback for consideration.

@johkin johkin requested a review from shakuzen November 15, 2024 11:33
@johkin
Copy link
Author

johkin commented Nov 18, 2024

I did some minor restructuring of the code to improve readability and error-handling/logging.

The method used to find the root element is identical in both interceptors, could be and improvement to refactor this to a helper-class? On the other hand, this requires the helper to be injected in the interceptors and creates another dependency.

@johkin johkin requested a review from shakuzen November 19, 2024 15:33
* DefaultHandler that extracts the root elements namespace and name.
* @author Johan Kindgren
*/
public class RootElementSAXHandler extends DefaultHandler {
Copy link
Member

Choose a reason for hiding this comment

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

Should this be public or can it be package-private?

Copy link
Member

Choose a reason for hiding this comment

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

Oops. I was looking at old commits.


private final SAXParser saxParser;

public ObservationHelper() {
Copy link
Member

@shakuzen shakuzen Nov 26, 2024

Choose a reason for hiding this comment

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

Is there a reason to have multiple instances of this class? Could it be a singleton utility class instead?

Copy link
Member

@shakuzen shakuzen Nov 26, 2024

Choose a reason for hiding this comment

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

Or alternatively, could the public method be static and it not have a public constructor?

Copy link
Author

Choose a reason for hiding this comment

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

It could certainly be a singleton, is something like this what you had in mind:
`java

private static ObservationHelper INSTANCE;

public static ObservationHelper getInstance() {
    if (INSTANCE == null) {
        INSTANCE = new ObservationHelper();
    }
    return INSTANCE;
}

private ObservationHelper() {

`

Also noted that a minor bug slipped through the tests, I have a fix in place locally.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, that would be one option. I'll leave it up to @corneil to advise on what makes most sense given existing patterns used in this codebase.

@corneil
Copy link
Contributor

corneil commented Nov 27, 2024

@johkin when I checkout the PR and run ./mvnw clean package there are test failures:

[ERROR] Failures: 
[ERROR]   WebServiceTemplateObservationIntegrationTest.attachment:305 There should be at least one Observation that matches the assertion. Found following Observations:
name='webservice.client', contextualName='POST', error='null', lowCardinalityKeyValues=[exception='none', host='localhost', localpart='unknow', namespace='unknown', outcome='success', soapaction='none'], highCardinalityKeyValues=[path='/soap/attachment'], map=[], parentObservation=null
[ERROR]   WebServiceTemplateObservationIntegrationTest.marshalSendAndReceiveNoResponse:222 There should be at least one Observation that matches the assertion. Found following Observations:
name='webservice.client', contextualName='POST', error='null', lowCardinalityKeyValues=[exception='none', host='localhost', localpart='unknow', namespace='unknown', outcome='success', soapaction='none'], highCardinalityKeyValues=[path='/soap/noResponse'], map=[], parentObservation=null
[ERROR]   WebServiceTemplateObservationIntegrationTest.notFound:241 There should be at least one Observation that matches the assertion. Found following Observations:
name='webservice.client', contextualName='POST', error='org.springframework.ws.client.WebServiceTransportException: Server Error [500]', lowCardinalityKeyValues=[exception='WebServiceTransportException', host='localhost', localpart='unknow', namespace='unknown', outcome='fault', soapaction='none'], highCardinalityKeyValues=[path='/errors/notfound'], map=[], parentObservation=null
[ERROR]   WebServiceTemplateObservationIntegrationTest.receiverFault:262 There should be at least one Observation that matches the assertion. Found following Observations:
name='webservice.client', contextualName='POST', error='org.springframework.ws.soap.client.SoapFaultClientException: Receiver Fault', lowCardinalityKeyValues=[exception='SoapFaultClientException', host='localhost', localpart='unknow', namespace='unknown', outcome='fault', soapaction='none'], highCardinalityKeyValues=[path='/soap/receiverFault'], map=[], parentObservation=null
[ERROR]   WebServiceTemplateObservationIntegrationTest.sendSourceAndReceiveToResult:158 There should be at least one Observation that matches the assertion. Found following Observations:
name='webservice.client', contextualName='POST', error='null', lowCardinalityKeyValues=[exception='none', host='localhost', localpart='unknow', namespace='unknown', outcome='success', soapaction='none'], highCardinalityKeyValues=[path='/soap/echo'], map=[], parentObservation=null
[ERROR]   WebServiceTemplateObservationIntegrationTest.sendSourceAndReceiveToResultNoResponse:177 There should be at least one Observation that matches the assertion. Found following Observations:
name='webservice.client', contextualName='POST', error='null', lowCardinalityKeyValues=[exception='none', host='localhost', localpart='unknow', namespace='unknown', outcome='success', soapaction='none'], highCardinalityKeyValues=[path='/soap/noResponse'], map=[], parentObservation=null
[ERROR]   WebServiceTemplateObservationIntegrationTest.senderFault:282 There should be at least one Observation that matches the assertion. Found following Observations:
name='webservice.client', contextualName='POST', error='org.springframework.ws.soap.client.SoapFaultClientException: Sender Fault', lowCardinalityKeyValues=[exception='SoapFaultClientException', host='localhost', localpart='unknow', namespace='unknown', outcome='fault', soapaction='none'], highCardinalityKeyValues=[path='/soap/senderFault'], map=[], parentObservation=null
[ERROR]   ObservationInterceptorIntegrationTest.testObservationInterceptorBehavior:102 There should be at least one Observation that matches the assertion. Found following Observations:
name='webservice.server', contextualName='POST /ws/{pathInfo}', error='null', lowCardinalityKeyValues=[exception='none', localpart='unknow', namespace='unknown', outcome='success', path='/ws/{pathInfo}', soapaction='none'], highCardinalityKeyValues=[pathinfo='/1234'], map=[], parentObservation=null,name='webservice.server', contextualName='POST /ws', error='null', lowCardinalityKeyValues=[exception='none', localpart='unknow', namespace='unknown', outcome='success', path='/ws', soapaction='none'], highCardinalityKeyValues=[], map=[], parentObservation=null
[ERROR]   ObservationInterceptorIntegrationTest.testPathWithVariable:125 There should be at least one Observation that matches the assertion. Found following Observations:
name='webservice.server', contextualName='POST /ws/{pathInfo}', error='null', lowCardinalityKeyValues=[exception='none', localpart='unknow', namespace='unknown', outcome='success', path='/ws/{pathInfo}', soapaction='none'], highCardinalityKeyValues=[pathinfo='/1234'], map=[], parentObservation=null

@johkin
Copy link
Author

johkin commented Nov 27, 2024

Yes, I noticed the test-failures yesterday. I've updated the code to handle the issue correctly.

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.

4 participants