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

PAYARA-4123 Refactored server JAXRS Request Tracing #4219

Merged

Conversation

@dmatej
Copy link
Contributor

commented Sep 16, 2019

  • removed filter + exception mapper solution, which failed TCK's completitioncallback tests
  • introduced EventListener solution, depends on Jersey
  • removed trailing whitespaces in the module
  • added logging and several generics
  • reduced nesting of blocks in favor of readibility
  • EnvironmentConfigSource uses AccessController to avoid security exceptions
@dmatej dmatej requested review from Cousjava, smillidge and Pandrex247 Sep 16, 2019
@dmatej dmatej self-assigned this Sep 16, 2019
@dmatej

This comment has been minimized.

Copy link
Contributor Author

commented Sep 16, 2019

Jenkins test please

Copy link
Member

left a comment

Haven't fully reviewed the code yet, but made some observations.

I'm not convinced this works quite right - or it at least works differently.
I get different trace outcomes from simply enabling request tracing with the following settings:

  • Applications Only = Disabled
  • Threshold Unit = Milliseconds
  • Selected Notifiers = service-log

https://gist.github.com/Pandrex247/fde4ed9fa2c813e2a4359e8f2dca18b9
https://gist.github.com/Pandrex247/80d0b0dda3f8419877d778cc7baed7a4

I also get an IOException when running the default endpoint of the following app from a blog I did a while ago, with it claiming that the active scope is null due it already being shut, something that worked previously. The example is a bit weird admittedly, but should probably be looked at.

opentracing.zip

Also also, I'm getting these log messages printed out which look a bit like debug statements:
onEvent(event.type=MATCHING_START)
onEvent(event.type=REQUEST_FILTERED)

@Pandrex247

This comment has been minimized.

Copy link
Member

commented Sep 17, 2019

endpoint4 of the example app was just me messing around btw and wasn't included in the blog - I don't think that one works.

@dmatej dmatej requested a review from Pandrex247 Sep 17, 2019
@dmatej

This comment has been minimized.

Copy link
Contributor Author

commented Sep 17, 2019

Jenkins test please

Copy link
Member

left a comment

NPE when enabling request tracing from admin console:

  • Enable request tracing, Threshold Unit Microseconds, Applications Only disabled, service-log notifier enabled.
  • Values save successfully but NPE in logs:
java.lang.NullPointerException
        at fish.payara.notification.requesttracing.RequestTrace.assignReferences(RequestTrace.java:307)
        at fish.payara.notification.requesttracing.RequestTrace.endTrace(RequestTrace.java:184)
        at fish.payara.nucleus.requesttracing.RequestTraceSpanStore.endTrace(RequestTraceSpanStore.java:88)
        at fish.payara.nucleus.requesttracing.RequestTracingService.endTrace(RequestTracingService.java:428)
@dmatej

This comment has been minimized.

Copy link
Contributor Author

commented Sep 18, 2019

&& span.getTraceEndTime().compareTo(comparisonSpan.getTraceEndTime()

Could it be currently processed request?

Copy link
Member

left a comment

When combined with #4234 everything seems to work

@Pandrex247

This comment has been minimized.

Copy link
Member

commented Sep 20, 2019

I didn't get conflicts when I merged them locally...

@dmatej dmatej referenced this pull request Sep 23, 2019
3 of 9 tasks complete
dmatej added 2 commits Sep 16, 2019
- removed filter + exception mapper solution, which failed
  TCK's completitioncallback tests
- introduced EventListener solution, depends on Jersey
- removed trailing whitespaces in the module
- added logging and several generics
- reduced nesting of blocks in favor of readibility
- EnvironmentConfigSource uses AccessController to avoid security exceptions
- tracedAnnotation instance cannot be resolved if the event is for example
  ON_EXCEPTION invoked sooner than after REQUEST_MATCHED event.
- how to simulate: invoke unmapped context causing HTTP 404.
@dmatej

This comment has been minimized.

Copy link
Contributor Author

commented Sep 23, 2019

It depends on merging strategy and GIT settings - it detected "file copy" but it was false positive. I will push rebased version after TCK tests ...

@dmatej dmatej force-pushed the dmatej:PAYARA-4123-fixed-completioncallback-refactored branch from 4770b0e to 0fc58a2 Sep 23, 2019
@dmatej

This comment has been minimized.

Copy link
Contributor Author

commented Sep 23, 2019

Jenkins test please

@dmatej dmatej merged commit 603150a into payara:master Sep 24, 2019
24 of 28 checks passed
24 of 28 checks passed
security/snyk - appserver/distributions/pom.xml (payara-ci) Test in progress
security/snyk - nucleus/core/pom.xml (payara-ci) Test in progress
security/snyk - nucleus/grizzly/pom.xml (payara-ci) Test in progress
security/snyk - nucleus/resources-l10n/pom.xml (payara-ci) Test in progress
Payara Quick Build and Test Quick build and test passed!
Details
security/snyk - api/payara-api/pom.xml (payara-ci) No new issues
Details
security/snyk - api/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/admingui/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/common/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/connectors/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/deployment/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/extras/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/featuresets/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/grizzly/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/installer/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/jms/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/load-balancer/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/orb/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/osgi-platforms/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/packager/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/registration/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/security/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/transaction/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/cluster/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/distributions/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/packager/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/resources/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/test-utils/pom.xml (payara-ci) No new issues
Details
@dmatej dmatej deleted the dmatej:PAYARA-4123-fixed-completioncallback-refactored branch Sep 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.